ESQL: Check field exists before load from _source #103632

nik9000 · 2023-12-20T17:08:07Z

Loading fields from _source is super slow because you have to decompress the stored fields and then turn the stored field into a map-of-maps. And then dig through the map-of-maps. This adds "does this field exist" style checks before most loads from _source. Not all fields can do it, but most fields can.

This really improves the performance of our
esql_dissect_duration_and_stats benchmark, mostly because it is running dissect on a field that has to load from _source that isn't in many of the documents. Here's the performance:

|  50th percentile service time | 867.667 | 100.491 | -767.176 | ms | -88.42% |
|  90th percentile service time | 886.042 | 102.434 | -783.608 | ms | -88.44% |
| 100th percentile service time | 893.035 | 104.598 | -788.437 | ms | -88.29% |

Loading fields from `_source` is *super* slow because you have to decompress the stored fields and then turn the stored field into a map-of-maps. And then dig through the map-of-maps. This adds "does this field exist" style checks before most loads from `_source`. Not all fields can do it, but most fields can. This really improves the performance of our `esql_dissect_duration_and_stats` benchmark, mostly because it is running `dissect` on a field that has to load from `_source` that isn't in many of the documents. Here's the performance: ``` | 50th percentile service time | 867.667 | 100.491 | -767.176 | ms | -88.42% | | 90th percentile service time | 886.042 | 102.434 | -783.608 | ms | -88.44% | | 100th percentile service time | 893.035 | 104.598 | -788.437 | ms | -88.29% | ```

elasticsearchmachine · 2023-12-20T17:08:31Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-12-20T17:08:31Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

elasticsearchmachine · 2023-12-20T17:08:31Z

Hi @nik9000, I've created a changelog YAML for you.

…nto text_load_norms

costin · 2023-12-21T03:59:31Z

Great stuff - I wonder if this is something we can do at the planner level without affecting the other columns.

dnhatn

I left a comment, but this is neat. Thanks Nik!

dnhatn · 2023-12-21T07:00:04Z

server/src/main/java/org/elasticsearch/index/mapper/BlockSourceReader.java

+            public DocIdSetIterator lookup(LeafReaderContext ctx) throws IOException {
+                Terms terms = ctx.reader().terms(FieldNamesFieldMapper.NAME);
+                if (terms == null) {
+                    return null;


Should we return DocIdSetIterator.all(ctx.reader().maxDoc()) when the terms don't exist? I think we allow disabling the _field_names field on 7.x. However, we would lose the benefit for fields without values in a segment. We can check the created index version, but we don't have it here.

Oh boy! That's fun. Let me play a bit.

nik9000 · 2023-12-21T13:05:58Z

Great stuff - I wonder if this is something we can do at the planner level without affecting the other columns.

In some cases I think we can push the IS NOT NULL constraint all the way to Lucene. This'll still be helpful in the cases where we can't push it all the way. Like when we actually need the null-ness of the field for something.

elasticsearchmachine · 2024-01-02T19:49:08Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

nik9000 · 2024-01-04T21:48:31Z

@dnhatn I've got it! You were right, the field names field can be disabled, but it's not simple to do. It's kind of annoying to do in a unit test too, so I dropped it into an integration test and then fixed it.

dnhatn

LGTM. Thanks Nik for the extra iteration.

nik9000 · 2024-01-05T13:51:35Z

Thanks for pointing out the problem @dnhatn ! It'd be lame if indices upgraded from 7.x didn't work properly.

We had this muted before, then fixed it, and now muted it again after elastic#103632 (ESQL: Check field exists before load from _source)

* ESQL reading points from source Update docs/changelog/102880.yaml Make SpatialPoint a concrete class Simplifies code in a few places, and is a step towards easier implementation of other features, especially CRS support and ESQL support. Continued work on point block support Spotless checks for generated code Disable test for POINT in a few complex cases * MultivalueDedupeTests seems to order POINTs incorrectly * TopNOperatorTests might need to exclude POINT always unless we want to support POINT in TopN results * BlockHashRendomizedTests has failures on POINT, perhaps an issue with different hashcode calculations? Added missing generated code for building point types Get CsvTests working with more support for PointBlock Fixed ToString and ToLong tests after change of point from encoded Added missing new generated files Updated changelog to be ESQL specific. A hint as to where to tie into the planner for doc-values vs source Small refinement to PointArray/Vector memory estimates Small refinement to PointArray/Vector memory estimates Small refinement to PointArray/Vector memory estimates Reorganize BlockFectory to improve symmetry between Double and Point Support read/write Point in StreamInput/Output Fix some more tests Fixed asymmetry between Geo and Cartesian field types This was expressed as test failures for CartesianPoint since it was still reading from doc-values instead of from source, while the rest of the stack expected source. Disable failing field type tests for now Currently reading from source for GeoPointFieldMapper is not properly supported in the tests Fixed failing tests with unsupported types and precision change Fix tests that still used longs for spatial points Added source values to spatial types in textFormatting tests Fixed EsqlQueryResponse to map point back to geo_point after deserialization Fixed binary comparison tests for spatial types In particular, we do not support inequality operators on spatial types. For example: point < point, is not a meaningful expression. Fixed mixed-cluster tests for csv-tests and spatial precision Fixed mixed-cluster tests for esql types and point precision More tests and use warning for parse error Remove SpatialPoint from StreamInput/Output Added fromStats() for spatial block values from doc-values Serialize GeoPoint and SpatialPoint in ESQL plan Retain point types in plan serialization Fixup after rebase * Removed support for unsigned_long from ToGeoPoint and ToCartesianPoint, since the mapping from unsigned_long to long allows for invalid values * Support new X-BigArrayBlock introduced by Nhat Fix test where there can be duplicated hashcodes When cartesian decoding fails, we should test and mimic that failure * Initial support for BytesRefBlock to support WKB for Geometries This commit does not yet remove support for PointBlock, since we are investigating how to kabe multiple block types backing the same geometries. * Update docs/changelog/103698.yaml * Fixed changelog files * Remove support for PointBlock We will use only WKB encoded in BytesRefBlock for now * One change from code review * Removed some leftovers from the PointBlock removal * Some code-review and TODO checks * Some code-review and TODO checks, and deal with Plan serialization For plan serialization we needed to care that 8.12 expected point literals to serialize as encoded long values. * Fixed failing tests with point WKT rendering * Revert stab at implementing forStats for doc-values vs source We can try this again in another PR * Disabled failing test * Use max precision when serializing points to XContent * Removed unused class from PointBlock code * Simplifications from code review * Remove intermediate WKT from GeoPointFieldMapper Previously we were reading source, mapping to WKT and then later mapping to WKB. Now just map directly to WKB. * Refactor WKBTopNEncoder to use length prefix encoding The previous approach of using WKT was expensive and wasteful. Since the types are unsortable, we can just encode them as is. * Fix failing test * Remove WKBTopNEncoder since DefaultUnsortableTopNEncoder can do the job We move the support for BytesRef encoding to DefaultUnsortableTopNEncoder. * Cleanup from code review * Remove concrete SpatialPoint class SpatialPoint reverts to an interface * Bring back SpatialPointTests Even without a concrete SpatialPoint class, this test suite has some utility. It asserts, for example, that GeoPoint and other implementations of SpatialPoint do not have the same hash-code even if they contain the same coordinates. Likewise the equals method is tested. These tests are valid regardless of whether GeoPoint and CartesianPoint share a common parent class, or merely implement an interface. * Reduce object creation in some tests * Removed unused code * Removed unused code from earlier versions Earlier versions of point support needed this, but it is no longer needed. * Tests were testing cases that no longer apply We moved internal types to WKB, so testing points themselves no longer makes sense. * Tests were testing cases that no longer apply We moved internal types to WKB, so testing points themselves no longer makes sense. * We need to consider WKB in PlanNamedTypes mixed-cluster * Removed SpatialPoint from PlanNamedTypes Only tests were passing spatial points in here, and those have been fixed to generate WKB, which is what is used in production code. * Removed one more artifact of using SpatialPoint in tests * Some work towards removing creating SpatialPoints * Updated GeoPointFieldMapperTests after merge from main * Get row-stride-reader test working Based on Niks support for not using synthetic source, but also by using WKT instead of WKB for test assertions. This is partly because it is easier to debug, but also because the test code uses base64 for encoding expected values, and the production code does not. Switching to WKT avoids that test code pitfall. * Geo flaky tests to work more reliably * Removed defensive coding in plan serialization * Simplify and ensure error is thrown on wrong type * Do version checks on reading plan from PlanStreamInput * Mute failing test We had this muted before, then fixed it, and now muted it again after #103632 (ESQL: Check field exists before load from _source) * Use specific TransportVersions for point literal in query plans Since we might start testing ESQL in serverless any day now, this is a better option, as the previous approach was more stateful (relied on 8.12.x vs. 8.13.x). * Fixed failing test after merge with main The code in main was using an optimization that is not supported by GeoPointFieldMapper, but could be done as a future improvement. * Fixed failing test after merge with main, with nullValue from source The nullValue is injected into source fetching in such a way that is is epxected to be in source format. So we need to convert it back to a source format. We picked WKT because that is clear and simple in the debugger, but GeoJSON and an object map of lat/long (for geoPoint only) worked too. Curiously a double[]{x,y} did not work, even though it is a valid source format. We did not investigate why. * Fixed compile error from previous fix with nullValues * It seems mixed cluster yaml tests don't work See #103947 --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Loading fields from `_source` is *super* slow because you have to decompress the stored fields and then turn the stored field into a map-of-maps. And then dig through the map-of-maps. This adds "does this field exist" style checks before most loads from `_source`. Not all fields can do it, but most fields can. This really improves the performance of our `esql_dissect_duration_and_stats` benchmark, mostly because it is running `dissect` on a field that has to load from `_source` that isn't in many of the documents. Here's the performance: ``` | 50th percentile service time | 867.667 | 100.491 | -767.176 | ms | -88.42% | | 90th percentile service time | 886.042 | 102.434 | -783.608 | ms | -88.44% | | 100th percentile service time | 893.035 | 104.598 | -788.437 | ms | -88.29% | ```

* ESQL reading points from source Update docs/changelog/102880.yaml Make SpatialPoint a concrete class Simplifies code in a few places, and is a step towards easier implementation of other features, especially CRS support and ESQL support. Continued work on point block support Spotless checks for generated code Disable test for POINT in a few complex cases * MultivalueDedupeTests seems to order POINTs incorrectly * TopNOperatorTests might need to exclude POINT always unless we want to support POINT in TopN results * BlockHashRendomizedTests has failures on POINT, perhaps an issue with different hashcode calculations? Added missing generated code for building point types Get CsvTests working with more support for PointBlock Fixed ToString and ToLong tests after change of point from encoded Added missing new generated files Updated changelog to be ESQL specific. A hint as to where to tie into the planner for doc-values vs source Small refinement to PointArray/Vector memory estimates Small refinement to PointArray/Vector memory estimates Small refinement to PointArray/Vector memory estimates Reorganize BlockFectory to improve symmetry between Double and Point Support read/write Point in StreamInput/Output Fix some more tests Fixed asymmetry between Geo and Cartesian field types This was expressed as test failures for CartesianPoint since it was still reading from doc-values instead of from source, while the rest of the stack expected source. Disable failing field type tests for now Currently reading from source for GeoPointFieldMapper is not properly supported in the tests Fixed failing tests with unsupported types and precision change Fix tests that still used longs for spatial points Added source values to spatial types in textFormatting tests Fixed EsqlQueryResponse to map point back to geo_point after deserialization Fixed binary comparison tests for spatial types In particular, we do not support inequality operators on spatial types. For example: point < point, is not a meaningful expression. Fixed mixed-cluster tests for csv-tests and spatial precision Fixed mixed-cluster tests for esql types and point precision More tests and use warning for parse error Remove SpatialPoint from StreamInput/Output Added fromStats() for spatial block values from doc-values Serialize GeoPoint and SpatialPoint in ESQL plan Retain point types in plan serialization Fixup after rebase * Removed support for unsigned_long from ToGeoPoint and ToCartesianPoint, since the mapping from unsigned_long to long allows for invalid values * Support new X-BigArrayBlock introduced by Nhat Fix test where there can be duplicated hashcodes When cartesian decoding fails, we should test and mimic that failure * Initial support for BytesRefBlock to support WKB for Geometries This commit does not yet remove support for PointBlock, since we are investigating how to kabe multiple block types backing the same geometries. * Update docs/changelog/103698.yaml * Fixed changelog files * Remove support for PointBlock We will use only WKB encoded in BytesRefBlock for now * One change from code review * Removed some leftovers from the PointBlock removal * Some code-review and TODO checks * Some code-review and TODO checks, and deal with Plan serialization For plan serialization we needed to care that 8.12 expected point literals to serialize as encoded long values. * Fixed failing tests with point WKT rendering * Revert stab at implementing forStats for doc-values vs source We can try this again in another PR * Disabled failing test * Use max precision when serializing points to XContent * Removed unused class from PointBlock code * Simplifications from code review * Remove intermediate WKT from GeoPointFieldMapper Previously we were reading source, mapping to WKT and then later mapping to WKB. Now just map directly to WKB. * Refactor WKBTopNEncoder to use length prefix encoding The previous approach of using WKT was expensive and wasteful. Since the types are unsortable, we can just encode them as is. * Fix failing test * Remove WKBTopNEncoder since DefaultUnsortableTopNEncoder can do the job We move the support for BytesRef encoding to DefaultUnsortableTopNEncoder. * Cleanup from code review * Remove concrete SpatialPoint class SpatialPoint reverts to an interface * Bring back SpatialPointTests Even without a concrete SpatialPoint class, this test suite has some utility. It asserts, for example, that GeoPoint and other implementations of SpatialPoint do not have the same hash-code even if they contain the same coordinates. Likewise the equals method is tested. These tests are valid regardless of whether GeoPoint and CartesianPoint share a common parent class, or merely implement an interface. * Reduce object creation in some tests * Removed unused code * Removed unused code from earlier versions Earlier versions of point support needed this, but it is no longer needed. * Tests were testing cases that no longer apply We moved internal types to WKB, so testing points themselves no longer makes sense. * Tests were testing cases that no longer apply We moved internal types to WKB, so testing points themselves no longer makes sense. * We need to consider WKB in PlanNamedTypes mixed-cluster * Removed SpatialPoint from PlanNamedTypes Only tests were passing spatial points in here, and those have been fixed to generate WKB, which is what is used in production code. * Removed one more artifact of using SpatialPoint in tests * Some work towards removing creating SpatialPoints * Updated GeoPointFieldMapperTests after merge from main * Get row-stride-reader test working Based on Niks support for not using synthetic source, but also by using WKT instead of WKB for test assertions. This is partly because it is easier to debug, but also because the test code uses base64 for encoding expected values, and the production code does not. Switching to WKT avoids that test code pitfall. * Geo flaky tests to work more reliably * Removed defensive coding in plan serialization * Simplify and ensure error is thrown on wrong type * Do version checks on reading plan from PlanStreamInput * Mute failing test We had this muted before, then fixed it, and now muted it again after elastic#103632 (ESQL: Check field exists before load from _source) * Use specific TransportVersions for point literal in query plans Since we might start testing ESQL in serverless any day now, this is a better option, as the previous approach was more stateful (relied on 8.12.x vs. 8.13.x). * Fixed failing test after merge with main The code in main was using an optimization that is not supported by GeoPointFieldMapper, but could be done as a future improvement. * Fixed failing test after merge with main, with nullValue from source The nullValue is injected into source fetching in such a way that is is epxected to be in source format. So we need to convert it back to a source format. We picked WKT because that is clear and simple in the debugger, but GeoJSON and an object map of lat/long (for geoPoint only) worked too. Curiously a double[]{x,y} did not work, even though it is a valid source format. We did not investigate why. * Fixed compile error from previous fix with nullValues * It seems mixed cluster yaml tests don't work See elastic#103947 --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

nik9000 added >enhancement :Analytics/ES|QL AKA ESQL v8.13.0 labels Dec 20, 2023

nik9000 requested review from dnhatn and craigtaverner December 20, 2023 17:08

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Dec 20, 2023

nik9000 added 3 commits December 20, 2023 12:08

Update docs/changelog/103632.yaml

eb3dce2

Fixup

bd43f29

Merge remote-tracking branch 'refs/remotes/nik9000/text_load_norms' i…

665a1b5

…nto text_load_norms

dnhatn reviewed Dec 21, 2023

View reviewed changes

Merge branch 'main' into text_load_norms

38f0cc0

wchaparro added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 2, 2024

elasticsearchmachine removed the Team:QL (Deprecated) Meta label for query languages team label Jan 2, 2024

nik9000 added 5 commits January 3, 2024 16:23

Merge branch 'main' into text_load_norms

fc20b5a

MErge

1b956bb

Merge branch 'main' into text_load_norms

ad13fd5

WIP

8fa4a43

Move fieldname

1afe278

Spelling

539758d

dnhatn approved these changes Jan 4, 2024

View reviewed changes

nik9000 merged commit a994aed into elastic:main Jan 5, 2024
15 checks passed

craigtaverner added a commit to craigtaverner/elasticsearch that referenced this pull request Jan 5, 2024

Mute failing test

d18e3a9

We had this muted before, then fixed it, and now muted it again after elastic#103632 (ESQL: Check field exists before load from _source)

nik9000 mentioned this pull request Jan 8, 2024

ESQL: Performance degradation in dissect benchmark #101997

Closed

gmarouli mentioned this pull request Jan 12, 2024

[CI] FullClusterRestartIT testDisableFieldNameField {cluster=OLD} failing #104101

Closed

nik9000 mentioned this pull request Jan 18, 2024

ESQL: Allow grouping by null blocks #104523

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Check field exists before load from _source #103632

ESQL: Check field exists before load from _source #103632

nik9000 commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

costin commented Dec 21, 2023

dnhatn left a comment

dnhatn Dec 21, 2023

nik9000 Dec 21, 2023

nik9000 commented Dec 21, 2023

elasticsearchmachine commented Jan 2, 2024

nik9000 commented Jan 4, 2024

dnhatn left a comment

nik9000 commented Jan 5, 2024

ESQL: Check field exists before load from _source #103632

ESQL: Check field exists before load from _source #103632

Conversation

nik9000 commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

elasticsearchmachine commented Dec 20, 2023

costin commented Dec 21, 2023

dnhatn left a comment

Choose a reason for hiding this comment

dnhatn Dec 21, 2023

Choose a reason for hiding this comment

nik9000 Dec 21, 2023

Choose a reason for hiding this comment

nik9000 commented Dec 21, 2023

elasticsearchmachine commented Jan 2, 2024

nik9000 commented Jan 4, 2024

dnhatn left a comment

Choose a reason for hiding this comment

nik9000 commented Jan 5, 2024