Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregator dependency injection, min/max, and skipNaN #1108

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Jolanrensen
Copy link
Collaborator

@Jolanrensen Jolanrensen commented Mar 25, 2025

Helps #961

  • Refactored Aggregator, splitting it into 3 parts:
    • Input, can be supplied by a class implementing AggregatorInputHandler, has 2 implementations
    • Aggregation, can be supplied by a class implementing AggregatorAggregationHandler, has 2 implementations
    • Handling multiple columns, can be supplied by a class implementing AggregatorMultipleColumnsHandler, has 3 implementations

Together they can instantiate Aggregator in any combination you like :)

  • Added Aggregator.aggregateByOrNull() functions for selecting aggregators, like min/max, but also median/percentile in the future, with the Sequence.bestBy family of internal functions

  • added skipNaN option to sum, mean, min, max

  • Rewrote min/max with correct types (T : Comparable<T & Any>?)

    • Decided against allowing mixed number types. Allowing this requires each function to get 3 overloads to avoid ambiguity:
      • <T : Comparable<T & Any>?> for normal comparables
      • <T : Number?> for mixed number types
      • <T> ... where T : Number?, T : Comparable<T & Any?> for normal numbers
    • Plus a slight rethink of the selecting aggregator because each min with numbers would then act like a minBy { it.toUnifiedNumber() } as it needs to return unconverted minimal value
    • But if it's valueble enough, we can add it
  • TODO remove skipNaN from min, max again; min nor max can ever return NaN, because any number compared to NaN is considered "better"

  • TODO More tests?

…egation of specific abilities of the aggregator to whichever combination the client chooses. And instead of having to create a new class for each combination, the same function can be used.
@Jolanrensen Jolanrensen mentioned this pull request Mar 25, 2025
9 tasks
@Jolanrensen Jolanrensen changed the title Another Aggregator refactor and support for min/max Aggregator dependency injection, min/max, and skipNaN Mar 28, 2025
@Jolanrensen Jolanrensen changed the title Aggregator dependency injection, min/max, and skipNaN Aggregator dependency injection, min/max, and skipNaN Mar 31, 2025
@Jolanrensen
Copy link
Collaborator Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant