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

refactor(search_family): Add Aggregator class #4290

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

BagritsevichStepan
Copy link
Contributor

@BagritsevichStepan BagritsevichStepan commented Dec 11, 2024

related to this comment

  1. Add Aggregator class.
  2. Remove redundant std::string copies entirely during the aggregation steps.

TODO:

  1. Add logs
  2. Remove the usage of std::function

BorysTheDev
BorysTheDev previously approved these changes Dec 11, 2024
Signed-off-by: Stepan Bagritsevich <[email protected]>
return descending;
}
return true; // Elements are equal
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we return true if elements are equal for less operator? As I understand you could take previous implementation and return something like "res != descending"

Copy link
Contributor Author

@BagritsevichStepan BagritsevichStepan Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense to return descending here. But just to note, we are not guaranteeing any strict ordering in the result when elements are equal. This is because the final order also depends on the initial distribution of keys across shards (results from shards are joined into one vector, and then a sort is performed on this array).

So, it is not a "stable" sort, meaning that the initial order of the data is influenced by the key distribution across shards, which may give the user the impression of an unstable sort

Upd.: After discussion, it was decided to return false if the elements are equal.

Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
@BagritsevichStepan BagritsevichStepan force-pushed the df.search/refactor-aggregator-logic branch from edb9ce4 to 34ea3c7 Compare December 12, 2024 10:55
@BagritsevichStepan
Copy link
Contributor Author

BagritsevichStepan commented Dec 12, 2024

I simplified the comparator logic, so I don't think it’s necessary to use std::less here as we discussed.

…ues is not present

Signed-off-by: Stepan Bagritsevich <[email protected]>
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.

2 participants