-
Notifications
You must be signed in to change notification settings - Fork 25.9k
ESQL: Support reading points from doc-values for STATS ST_CENTROID #104218
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
Changes from 36 commits
2536723
b80c495
5b12397
213c138
3c6731b
4297c93
09b3109
da0c312
4366bc2
a279e34
00dcdd7
07e16d1
a18aa6e
fe3452d
c61901d
68ad574
ec82452
6239f2f
1489df3
558c2ef
c884de2
b72b6d4
fdd5429
1fffba4
5ea442a
47f8cc3
efb091b
a3a2526
e89a2e4
918dfeb
8bde869
01f8eb5
4a75df2
4bc596a
ef1f214
1ecbdae
b760d19
9aed84e
a8ad9fd
f8c2890
90b500f
254b85b
8f4d00e
e1ea6d5
dcd137c
3e0a3c1
4997775
00d44e2
ee3ba02
ef5b7e4
12c6980
3646147
088cbf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,11 @@ public String indexName() { | |
| return "benchmark"; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean forStats() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Despite looking at the PR , I still find the method name confusing. |
||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public SearchLookup lookup() { | ||
| throw new UnsupportedOperationException(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 104218 | ||
| summary: "Support reading spatial points from doc-values for STATS" | ||
| area: "ES|QL" | ||
| type: enhancement | ||
| issues: [] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| [discrete] | ||
| [[esql-agg-st-centroid]] | ||
| === `ST_CENTROID` | ||
|
|
||
| Calculate the spatial centroid over a field with spatial point geometry type. | ||
|
|
||
| [source.merge.styled,esql] | ||
| ---- | ||
| include::{esql-specs}/spatial.csv-spec[tag=st_centroid-airports] | ||
| ---- | ||
| [%header.monospaced.styled,format=dsv,separator=|] | ||
| |=== | ||
| include::{esql-specs}/spatial.csv-spec[tag=st_centroid-airports-result] | ||
| |=== | ||
|
|
||
| Supported types: | ||
|
|
||
| include::types/st_centroid.asciidoc[] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| [%header.monospaced.styled,format=dsv,separator=|] | ||
| |=== | ||
| v | result | ||
| geo_point | geo_point | ||
| cartesian_point | cartesian_point | ||
| |=== |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [%header.monospaced.styled,format=dsv,separator=|] | ||
| |=== | ||
| v | result | ||
| cartesian_point | cartesian_point | ||
| keyword | cartesian_point | ||
| text | cartesian_point | ||
| |=== |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| [%header.monospaced.styled,format=dsv,separator=|] | ||
| |=== | ||
| v | result | ||
| geo_point | geo_point | ||
| keyword | geo_point | ||
| text | geo_point | ||
| |=== |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,7 +265,7 @@ protected IngestScriptSupport ingestScriptSupport() { | |
| } | ||
|
|
||
| @Override | ||
| protected Function<Object, Object> loadBlockExpected(MapperService mapper, String loaderFieldName) { | ||
| protected Function<Object, Object> loadBlockExpected() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small nit but you change the return type as well: protected Function<Object, BytesRef>
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow? I just reverted this method to the version before Luigi's PR. Neither his change nor mine changed the return type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just saying that it might be helpful to expose the type in the method signature |
||
| return v -> ((BytesRef) v).utf8ToString(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,5 +176,13 @@ protected AbstractPointFieldType( | |
| protected Object nullValueAsSource(T nullValue) { | ||
| return nullValue == null ? null : nullValue.toWKT(); | ||
| } | ||
|
|
||
| @Override | ||
| public BlockLoader blockLoader(BlockLoaderContext blContext) { | ||
| if (blContext.forStats() && hasDocValues()) { | ||
| return new BlockDocValuesReader.LongsBlockLoader(name()); | ||
| } | ||
| return blockLoaderFromSource(blContext); | ||
| } | ||
|
Comment on lines
+181
to
+188
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use the ternary operator to make this a one-liner: |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -649,6 +649,14 @@ public interface BlockLoaderContext { | |
| */ | ||
| String indexName(); | ||
|
|
||
| /** | ||
| * Whether the data will be used in stats. | ||
| * This is relevant to some types, where the choice between doc-values or reading from source is dependent on the usage. | ||
| * For example, spatial types use doc-values for stats, but read from source for search because doc-values are modified | ||
| * from the original. | ||
| */ | ||
| boolean forStats(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above - e.g.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is confusing for me here is that we'll set this value to false by default, and only set it to true for spatial aggregations loaded from fields. That makes complete sense for the spatial field mappers block loading, but all other types will get a BlockLoaderContext with The term All other types can say 'you don't want to use this for stats, well both doc-values and source are equally good enough for non-stats, so I'll load the one I think is fastest'. But if we call it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
|
|
||
| /** | ||
| * {@link SearchLookup} used for building scripts. | ||
| */ | ||
|
|
||
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.
We've considered alternative names, like
forDisplay(opposite of for stats), but every choice has pros and cons, so I've left it like this for now, since this most closely matches current needs.If we start using this for non-stats use cases, like spatial predicates that benefit from doc-values, we can change the name of the method at that point.
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.
Predicate and top-n run on the results of functions consuming the points will want it.