-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Draft: Fix value scripts in composite aggs #53232
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hit a pretty similar bug to this in #53315. I wonder if it'd be better to have a member on this class that is a
Function<BytesRef, BytesRef>
that does the normalization. That way we don't need to build theSortedBinaryDocValues
.SortedBinaryDocValues
can be a fairly heavy thing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option I see: a function similar to
docValuesFunc
that given aLeafReaderContext
returns a normalizer (Function<BytesRef, BytesRef>
). I can hook this into ValueSource, that's 1 level higher compared toSortedBinaryDocValues
.The benefit: the change is not spread all over the place, because ValueSource has all definitions in 1 file.
However, @not-napoleon is re-factoring this part of the code, it seems to me it's better to wait for this re-factoring and re-visit this problem after it's done, if we want to go this way. Maybe we even benefit from the re-factoring (or it gets harder, but I hope not ;-) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I think having the appropriate
reader
for this sort of thing inValuesSource
would be useful. Not all the time, but for composite and for the min and max optimizations and the like.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: The Values Source refactor barely touches
ValuesSource.java
. Most of the work happens inValuesSourceConfig
,ValuesSourceType
, andValueType
(and, like, 200 other files) If memory serves, the only change I made toValuesSource
was in #48320 which merged to master some time ago.Composite has sort of a parallel value system (e.g.
CompositeValuesSourceBuilder
,CompositeValuesSourceConfig
, &c) and we've been mostly ignoring that for the refactor. Eventually we'll need to reconcile them, but that's a ways off. In particular, there's a "legacy" version ofValuesSourceConfig#resolve()
that we'll be keeping in the refactor for now, which is whatCompositeValuesSourceBuilder
calls. So I would not expect the behavior to change significantly after the refactor.Also, we're pushing hard to get the VSRefactor into master soon, so if you want to wait on that, hopefully it won't be too long.
I haven't looked too deeply into this, so I'm not sure what our options are, but IMHO the closer to those composite-specific classes we can solve this, the better. It seems like this is a composite only problem (value scripts work correctly on non-composite aggs, AFAIK), which to me says composite is doing something wrong with how it handles the
WithScript
values source subclasses.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's composite only but with the special case of
match_all
:If you add a query it starts working again due to the different execution path.
Thanks for all the input! I will re-visit later and e.g. check if/how it conflicts with your re-factoring.