Refactoring to introduce calculated fields. - #3061
Open
fulmicoton wants to merge 2 commits into
Open
Conversation
Move ColumnBlockAccessor out of tantivy-columnar and into the aggregation implementation. Keep its behavior and tests intact while removing it from the public columnar API.
fulmicoton-dd
force-pushed
the
paul.masurel/refact-agg-for-calc-fields
branch
5 times, most recently
from
August 31, 2026 09:33
99fc37c to
5c77d94
Compare
fulmicoton
changed the title
fulmicoton
commented
Aug 31, 2026
| agg_data: &mut AggregationsSegmentCtx, | ||
| ) -> crate::Result<()> { | ||
| // TODO: remove once we fetch all values for all bucket ids in one go | ||
| if docs.len() == 1 && self.missing_u64.is_none() { |
Collaborator
Author
There was a problem hiding this comment.
The optimization has been pushed in <Column as BlockValueSource>::load_block or Column
fulmicoton-dd
force-pushed
the
paul.masurel/refact-agg-for-calc-fields
branch
2 times, most recently
from
August 31, 2026 10:11
fd2f136 to
d6ff44c
Compare
fulmicoton-dd
force-pushed
the
paul.masurel/refact-agg-for-calc-fields
branch
from
August 31, 2026 11:33
d6ff44c to
7d33d74
Compare
fulmicoton
marked this pull request as ready for review
August 31, 2026 15:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small refactoring to prepare for the introduction to calcualtd fields.
Instead of materializing a full Column, this PR tries to mildly tweak ColumnBlockAccessor so that it can be filled by different value sources.
The change was actually minor. Some of its accessor required passing a column object, but it was actually only to get cardinality information. The accessor object now stores the cardinality.
As part of the refacotring, I also renamed a bunch of struct and methods that are specific to fused term group bys to have a fused prefix, added assert to make sure we never build them with a column that is not full, and used the accessors specialized for full columns.
Also I removed the generics on ColumnBlockAccessor, as it is only used for u64 representation at the moment.