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

[DO NOT MERGE] make IndexRange agnostic of business logic #13442

Closed
wants to merge 1 commit into from

Conversation

sudeepdino008
Copy link
Member

@sudeepdino008 sudeepdino008 commented Jan 15, 2025

  • if we're going towards "register domains/ii from anywhere", should kv.Domain also be string? -- I guess it's not needed now though. We can achieve the target of "make agg free of business logic" with kv.Domain as int as well.
  • map feels ugly. can also just store the historyIdx (kv.InvertedIndex) in domain/history struct and iterate over to find the queried kv.InvertedIndex. But that seems inconsistent with what we did with aggregator.iis.

@sudeepdino008
Copy link
Member Author

sudeepdino008 commented Jan 15, 2025

actually can also consider ditching map (for agg.iis as well) ...just using array and iterating over it when we need a "search" a kv.Invertedindex. Some functions that suffer are: aggregatorrotx.IndexRange, aggregatorrotx.IntegrityInvertedIndexAllValuesAreInRange; but they were already using switch statement before (which is sort of involves checking all cases). Thoughts?

@AskAlexSharov
Copy link
Collaborator

actually can also consider ditching map (for agg.iis as well) ...just using array and iterating over it when we need a "search" a kv.Invertedindex. Some functions that suffer are: aggregatorrotx.IndexRange, aggregatorrotx.IntegrityInvertedIndexAllValuesAreInRange; but they were already using switch statement before (which is sort of involves checking all cases). Thoughts?

I would prioritize performance of BeginFilesRO() func. Because it’s very hot and will be used in places which may not query any indices

@sudeepdino008 sudeepdino008 changed the title make IndexRange agnostic of business logic [DO NOT MERGE] make IndexRange agnostic of business logic Jan 16, 2025
@sudeepdino008
Copy link
Member Author

closed in favor of #13459

@sudeepdino008 sudeepdino008 deleted the aggr9 branch January 16, 2025 10:38
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