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.
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
ESQL: ST_EXTENT_AGG optimize envelope extraction from doc-values for cartesian_shape #118802
ESQL: ST_EXTENT_AGG optimize envelope extraction from doc-values for cartesian_shape #118802
Changes from 6 commits
9ce03ad
a519779
d0c329f
7d75dd9
9055219
e61f7a8
753c8d7
f736839
b6ba18a
3f0b58f
47966cf
a8a2c95
4104176
05116c0
e89600b
e2743ae
72e3e34
d62e37d
f823bc5
71aadca
93e7129
48d8f94
58a491a
bc4a693
c6875bc
7aa6734
1d8ed8f
eff2f67
ebfd723
5553b68
a9d6027
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This does not really make sense to me. Both DOC_VALUES and EXTRACT_SPATIAL_BOUNDS will require a column reader. I suspect the situation is that the places this is used actually never test the EXTRACT_SPATIAL_BOUNDS case, so this does not matter. But once we add tests for that, it will.
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.
Yes, this is for backward compatibility. I'll just inline it and add a comment so it's clearer.
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.
This appear to be unused. I think we can remove this boolean from the enum entirely.
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.
Later I did see some code that could have used this, but at that point I also wondered if it would be better to just stick with the original boolean.
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.
Must be a left-over. Removed.
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.
Parameter not used
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.
Fixed.
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.
Nope. Still unused.
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.
Perhaps this should call generator?
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.
Yep :)
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 still does not call generator.
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.
What is the reason for this only looking at every second geometry?
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.
Verifying that the block loader extracts data from the correct docs.
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.
Printing the entire geometry in the assert string can be too verbose. These geometries can be very large. Also, what format is it printing in? WKT? If so, perhaps just truncating it to 200 characters would be good?
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.
Replaced with
Substring
.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 think this move makes things less clear. In most types there is a clear binary choice between row-reading and column-reading. Only for spatial types is there an actual fieldExtractPreference.
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.
Let's keep it this way and change it in the future, after we add more cases such as handling geo.
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.
This extra piece of code here makes things a little harder to understand. I think I liked it better the way it was before. Now it seems we this mapping between the boolean for column-vs-row to the fieldExtractPreference and vice versa in more places.
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.
Although, could you not just have columnReader = fieldExtractPreference.isColumnReader()? Having said that, the false on ExtractSpatialBounds seems strange.
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.
Same comment as above.
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.
Not really. My comment here was about continuing to use fieldExtractPreference, which we agreed on.
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.
However, I don't understand why
EXTRACT_SPATIAL_BOUNDS
has columnReader=false? It should be true, because we'll be reading doc-values, which is column oriented.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.
Well, it's mostly to avoid triggering the old behavior. Ideally nothing would use that Boolean, but since some old code does use it, I want this to take a different path.
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.
OK, upon re-reviewing the code, I see now what you mean. I've changed it to
EXTRACT_SPATIAL_BOUNDS
is column reader.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.
Perhaps this code is simply never tested, which is why we see no errors here?
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.
Yes, nothing is exercising the path of
EXTRACT_SPATIAL_BOUNDS
. I think I was trying to use it before I gave up on it and just testedAbstractShapeGeometryFieldMapper
directly! Let me revert this whole thing back to Boolean. No reason to have stamp coupling :)