ESQL: Correct out-of-range filter pushdowns #99961

alex-spies · 2023-09-27T14:37:32Z

Fix pushed down filters for binary comparisons that would implicitly compare a byte/short/int/long with an out of range value, like WHERE some_int_field < 1E300. Queries containing such comparisons lead to exceptions thrown by ElasticSearch. Instead, trivialize the query, either to a query that matches all single values or no values depending on the comparison.

This does not cover the same problem for half_float/scaled_float/float,
see #100130.

Closes #99960.

Prevent pushing down filters for binary comparisons that would implicitly compare a byte/short/int with an out of range value. This leads to exceptions thrown by Lucene - instead, evaluate the filter in ESQL only. This does not cover the same problem for half_float/scaled_float/float, see elastic#100130. Closes elastic#99960.

alex-spies · 2023-10-02T11:12:38Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java

@@ -194,6 +196,8 @@ private static Set<Attribute> missingAttributes(PhysicalPlan p) {
    }

    public static class PushFiltersToSource extends OptimizerRule<FilterExec> {
+        private static int HALF_FLOAT_MAX = 65504;


I did not find this constant defined in the code base - anyone know if it is defined somewhere, after all?

elasticsearchmachine · 2023-10-02T11:14:34Z

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

elasticsearchmachine · 2023-10-02T11:14:35Z

Hi @alex-spies, I've created a changelog YAML for you.

elasticsearchmachine · 2023-10-02T11:14:35Z

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

costin

I'm not sure this is the correct solution in the long term - emp_no is an integer so the comparison fails because the value that it is compared against is out of range.
Doing the comparison in ESQL happens because we're widening the types but we sacrifice performance.

The issue boils down to whether or not the widening should take place or not - inside filters I would argue this should translate into a NoMatch since the data is out of range.
In other words the type should be narrowed and, in case of failure, resolve the filter to false.

alex-spies · 2023-10-04T08:07:28Z

I'm not sure this is the correct solution in the long term - emp_no is an integer so the comparison fails because the value that it is compared against is out of range. Doing the comparison in ESQL happens because we're widening the types but we sacrifice performance.

The issue boils down to whether or not the widening should take place or not - inside filters I would argue this should translate into a NoMatch since the data is out of range. In other words the type should be narrowed and, in case of failure, resolve the filter to false.

Agree, long term we'd expect the filters to become trivial if we compare to out-of-range values. That should probably go into some other optimization rule, though, or into the filter logic itself.

My (short-term) goal with this PR is to not fail the queries since I think they are valid per ES|QL's semantics. This is not just about widening, but about allowing comparisons between different numerical types. ... where integer == 1.0 is a valid query, and it's unexpected that ... where integer == 1000000000000 is not.

…-in-lucene-filter

elasticsearchmachine · 2024-01-02T19:52:13Z

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

…-in-lucene-filter

elasticsearchmachine · 2024-01-23T19:39:03Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

alex-spies · 2024-01-23T19:39:09Z

Tests done, this should be ready for another round of reviews now.

costin · 2024-01-24T00:08:11Z

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

+        try {
+            InputStream mappingPropertiesStream = RestEsqlTestCase.class.getResourceAsStream("/mapping-all-types.json");
+            String properties = new String(mappingPropertiesStream.readAllBytes(), StandardCharsets.UTF_8);
+            MAPPING_ALL_TYPES = "{\"mappings\": " + properties + "}";
+        } catch (IOException ex) {
+            throw new RuntimeException(ex);
+        }


You need to close the stream in case of errors - hence use the try-with-resource pattern.
Also extract the code into a static method and use that for field initialization:

@BeforeClass public static void initClass() { MAPPING_ALL_TYPES = loadResourceAsString("/mapping-all-types.json"); } public static String loadResourceAsString(String name) throws Exception { try (var url = RestEsqlTestCase.class.getResource(name)) { return Files.readString(Paths.get(url.toURI()), StandardCharsets.UTF_8)); }

There's probably an utility somewhere in ES that does something similar.

🤦 of course, thank you.

I couldn't find something that just loads a file as text; most utilities seem to directly parse that (e.g. EsqlTestUtils.loadmapping and TypesTests.loadMapping) or not expose that (e.g. GeometrySimplifierTests.loadJsonFile) :/

costin

Thanks - this looks good.
Left some comments around style and unnecessary delegation.

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

costin · 2024-01-24T00:30:47Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlTranslatorHandler.java

@@ -27,7 +27,7 @@
 public final class EsqlTranslatorHandler extends QlTranslatorHandler {
    @Override
    public Query asQuery(Expression e) {
-        return ExpressionTranslators.toQuery(e, this);
+        return ExpressionTranslators.toQuery(e, this, EsqlQueryTranslators.QUERY_TRANSLATORS);


This is going to be problematic since you might run the same translation twice - instead iterate through the local translators and null is returned, then delegate to original ExpressionTranslators.

I see that in the meantime, Luigi's =~ PR added ESQL-specific query translators - I'll align with the now existing code and will move my code next to his.

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

…-in-lucene-filter

astefan

I've left few more clean up comments, otherwise LGTM

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlTranslatorHandler.java

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

costin

Need a bit more polishing when it comes to delegating.

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

costin · 2024-01-30T01:24:54Z

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

+                );
+                var result = runEsql(query, expectedWarnings, mode);
+
+                var values = (ArrayList) result.get("values");


Instead of doing casting import the as method from EsqTestUtils:
var values = as(result.get("values"), List.class)

Will do! EsqlUtils requires to re-export esql in Gradle, otherwise the class won't load - but that's a small change.

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

costin · 2024-01-30T01:38:22Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlTranslatorHandler.java

 import java.util.List;
 import java.util.function.Supplier;

+import static org.elasticsearch.xpack.ql.type.DataTypes.UNSIGNED_LONG;
+import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
+
 public final class EsqlTranslatorHandler extends QlTranslatorHandler {

    public static final List<ExpressionTranslator<?>> QUERY_TRANSLATORS = List.of(


This PR mixes ExpressionTranslators (which is a list of translators) with TranslatorHandler which is the pluggable component - namely the only thing that needs to be implemented.

Extract the list and the TrivialBinaryComparisons to a separate EsqlExpressionTranslators and have the EsqlTranslatorHandler delegate to it.

I agree - I initially had the new translator in a dedicated EsqlExpressionTranslators, but then moved it here to align with code that was added in the meantime.

More specifically, EsqlTranslatorHandler already contains EqualsIgnoreCaseTranslator. I can move both into a dedicated EsqlExpressionTranslators, but I'd do so in a separate PR to avoid touching the EqualsIgnoreCaseTranslator.

Is that good with you?

costin · 2024-01-30T01:39:35Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlTranslatorHandler.java

 public final class EsqlTranslatorHandler extends QlTranslatorHandler {

    public static final List<ExpressionTranslator<?>> QUERY_TRANSLATORS = List.of(
        new EqualsIgnoreCaseTranslator(),
+        new TrivialBinaryComparisons(),
        new ExpressionTranslators.BinaryComparisons(),


Is the BinaryComparison actually used? Isn't TrivialBinaryComparison picked up instead? Why not delegate from that to BinaryComparison?

If TrivialBinaryComparison doesn't apply, it returns null, and is thus skipped. Then BinaryComparison applies; I

Equivalently, I can replace both by a single translator that delegates to BinaryComparison if out-of-range comparisons don't apply. Should I go for it?

(Also, lots of local physical optimizer tests fail without BinaryComparison.)

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

…-in-lucene-filter

...ingle-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/RestEsqlIT.java

alex-spies · 2024-01-31T11:07:22Z

Thanks for reviews, Andrei and Costin!

alex-spies · 2024-01-31T11:10:44Z

And thanks for the super fast review of the build.gradle change, Rene!

* Change release version lookup to an instance method (#104902) * Upgrade to Lucene 9.9.2 (#104753) This commit upgrades to Lucene 9.9.2. * Improve `CANNOT_REBALANCE_CAN_ALLOCATE` explanation (#104904) Clarify that in this situation there is a rebalancing move that would improve the cluster balance, but there's some reason why rebalancing is not happening. Also points at the `can_rebalance_cluster_decisions` as well as the node-by-node decisions since the action needed could be described in either place. * Get from translog fails with large dense_vector (#104700) This change fixes the engine to apply the current codec when retrieving documents from the translog. We need to use the same codec than the main index in order to ensure that all the source data is indexable. The internal codec treats some fields differently than the default one, for instance dense_vectors are limited to 1024 dimensions. This PR ensures that these customizations are applied when indexing document for translog retrieval. Closes #104639 Co-authored-by: Elastic Machine <[email protected]> * [Connector Secrets] Add delete API endpoint (#104815) * Add DELETE endpoint for /_connector/_secret/{id} * Add endpoint to write_connector_secrets cluster privilege * Merge Aggregations into InternalAggregations (#104896) This commit merges Aggregations into InternalAggregations in order to remove the unnecessary hierarchy. * [Profiling] Simplify cost calculation (#104816) * [Profiling] Add the number of cores to HostMetadata * Update AWS pricelist (remove cost_factor, add usd_per_hour) * Switch cost calculations from 'cost_factor' to 'usd_per_hour' * Remove superfluous CostEntry.toXContent() * Check for Number type in CostEntry.fromSource() * Add comment * Retry get_from_translog during relocations (#104579) During a promotable relocation, a `get_from_translog` sent by the unpromotable shard to handle a real-time get might encounter `ShardNotFoundException` or `IndexNotFoundException`. In these cases, we should retry. This is just for `GET`. I'll open a second PR for `mGET`. The relevant IT is in the Stateless PR. Relates ES-5727 * indicating fix for 8.12.1 for int8_hnsw (#104912) * Removing the assumption from some tests that the request builder's request() method always returns the same object (#104881) * [DOCS] Adds get setting and update settings asciidoc files to security API index (#104916) * [DOCS] Adds get setting and update settings asciidoc files to security API index. * [DOCS] Fixes references in docs. * Reuse APMMeterService of APMTelemetryProvider (#104906) * Mute more tests that tend to leak searchhits (#104922) * ESQL: Fix SearchStats#count(String) to count values not rows (#104891) SearchStats#count incorrectly counts the number of documents (or rows) in which a document appears instead of the actual number of values. This PR fixes this by looking at the term frequency instead of the doc count. Fix #104795 * Adding request source for cohere (#104926) * Fixing a broken javadoc comment in ReindexDocumentationIT (#104930) This fixes a javadoc comment that was broken by #104881 * Fix enabling / disabling of APM agent "recording" in APMAgentSettings (#104324) * Add `type` parameter support, for sorting, to the Query API Key API (#104625) This adds support for the `type` parameter, for sorting, to the Query API key API. The type for an API Key can currently be either `rest` or `cross_cluster`. This was overlooked in #103695 when support for the `type` parameter was first introduced only for querying. * Apply publish plugin to es-opensaml-security-api project (#104933) * Support `match` for the Query API Key API (#104594) This adds support for the `match` query type to the Query API key Information API. Note that since string values associated to API Keys are mapped as `keywords`, a `match` query with no analyzer parameter is effectively equivalent to a `term` query for such fields (e.g. `name`, `username`, `realm_name`). Relates: #101691 * [Connectors API] Relax strict response parsing for get/list operations (#104909) * Limit concurrent shards per node for ESQL (#104832) Today, we allow ESQL to execute against an unlimited number of shards concurrently on each node. This can lead to cases where we open and hold too many shards, equivalent to opening too many file descriptors or using too much memory for FieldInfos in ValuesSourceReaderOperator. This change limits the number of concurrent shards to 10 per node. This number was chosen based on the _search API, which limits it to 5. Besides the primary reason stated above, this change has other implications: We might execute fewer shards for queries with LIMIT only, leading to scenarios where we execute only some high-priority shards then stop. For now, we don't have a partial reduce at the node level, but if we introduce one in the future, it might not be as efficient as executing all shards at the same time. There are pauses between batches because batches are executed sequentially one by one. However, I believe the performance of queries executing against many shards (after can_match) is less important than resiliency. Closes #103666 * [DOCS] Support for nested functions in ES|QL STATS...BY (#104788) * Document nested expressions for stats * More docs * Apply suggestions from review - count-distinct.asciidoc - Content restructured, moving the section about approximate counts to end of doc. - count.asciidoc - Clarified that omitting the `expression` parameter in `COUNT` is equivalent to `COUNT(*)`, which counts the number of rows. - percentile.asciidoc - Moved the note about `PERCENTILE` being approximate and non-deterministic to end of doc. - stats.asciidoc - Clarified the `STATS` command - Added a note indicating that individual `null` values are skipped during aggregation * Comment out mentioning a buggy behavior * Update sum with inline function example, update test file * Fix typo * Delete line * Simplify wording * Fix conflict fix typo --------- Co-authored-by: Liam Thompson <[email protected]> Co-authored-by: Liam Thompson <[email protected]> * [ML] Passing input type through to cohere request (#104781) * Pushing input type through to cohere request * switching logic to allow request to always override * Fixing failure * Removing getModelId calls * Addressing feedback * Switching to enumset * [Transform] Unmute 2 remaining continuous tests: HistogramGroupByIT and TermsGroupByIT (#104898) * Adding ActionRequestLazyBuilder implementation of RequestBuilder (#104927) This introduces a second implementation of RequestBuilder (#104778). As opposed to ActionRequestBuilder, ActionRequestLazyBuilder does not create its request until the request() method is called, and does not hold onto that request (so each call to request() gets a new request instance). This PR also updates BulkRequestBuilder to inherit from ActionRequestLazyBuilder as an example of its use. * Update versions to skip after backport to 8.12 (#104953) * Update/Cleanup references to old tracing.apm.* legacy settings in favor of the telemetry.* settings (#104917) * Exclude tests that do not work in a mixed cluster scenario (#104935) * ES|QL: Improve type validation in aggs for UNSIGNED_LONG and better support for VERSION (#104911) * [Connector API] Make update configuration action non-additive (#104615) * Save allocating enum values array in two hot spots (#104952) Our readEnum code instantiates/clones enum value arrays on read. Normally, this doesn't matter much but the two spots adjusted here are visibly hot during bulk indexing, causing GBs of allocations during e.g. the http_logs indexing run. * ESQL: Correct out-of-range filter pushdowns (#99961) Fix pushed down filters for binary comparisons that compare a byte/short/int/long with an out of range value, like WHERE some_int_field < 1E300. * [DOCS] Dense vector element type should be float for OpenAI (#104966) * Fix test assertions (#104963) * Move functions that generate lucene geometries under a utility class (#104928) We have functions that generate lucene geometries scattered in different places of the code. This commit moves everything under a utility class. * fixing index versions --------- Co-authored-by: Simon Cooper <[email protected]> Co-authored-by: Chris Hegarty <[email protected]> Co-authored-by: David Turner <[email protected]> Co-authored-by: Jim Ferenczi <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Navarone Feekery <[email protected]> Co-authored-by: Ignacio Vera <[email protected]> Co-authored-by: Tim Rühsen <[email protected]> Co-authored-by: Pooya Salehi <[email protected]> Co-authored-by: Keith Massey <[email protected]> Co-authored-by: István Zoltán Szabó <[email protected]> Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Costin Leau <[email protected]> Co-authored-by: Jonathan Buttner <[email protected]> Co-authored-by: Albert Zaharovits <[email protected]> Co-authored-by: Mark Vieira <[email protected]> Co-authored-by: Jedr Blaszyk <[email protected]> Co-authored-by: Nhat Nguyen <[email protected]> Co-authored-by: Abdon Pijpelink <[email protected]> Co-authored-by: Liam Thompson <[email protected]> Co-authored-by: Liam Thompson <[email protected]> Co-authored-by: Przemysław Witek <[email protected]> Co-authored-by: Joe Gallo <[email protected]> Co-authored-by: Lorenzo Dematté <[email protected]> Co-authored-by: Luigi Dell'Aquila <[email protected]> Co-authored-by: Armin Braun <[email protected]> Co-authored-by: Alexander Spies <[email protected]> Co-authored-by: David Kyle <[email protected]>

elasticsearchmachine added the v8.11.0 label Sep 27, 2023

alex-spies mentioned this pull request Sep 27, 2023

ESQL: Comparing a large long to an int field fails when pushed to source #99960

Closed

alex-spies force-pushed the esql-fix-wrong-type-in-lucene-filter branch 3 times, most recently from 94d2395 to 63af4c4 Compare October 2, 2023 11:00

alex-spies force-pushed the esql-fix-wrong-type-in-lucene-filter branch from 63af4c4 to 19e80ea Compare October 2, 2023 11:08

alex-spies commented Oct 2, 2023

View reviewed changes

alex-spies added >bug :Analytics/ES|QL AKA ESQL labels Oct 2, 2023

alex-spies marked this pull request as ready for review October 2, 2023 11:14

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

Update docs/changelog/99961.yaml

ef60e3a

costin reviewed Oct 3, 2023

View reviewed changes

alex-spies added 3 commits October 4, 2023 11:51

Merge remote-tracking branch 'upstream/main' into esql-fix-wrong-type…

af03014

…-in-lucene-filter

Spotless

6750d70

Make Checkstyle happy

b9e751b

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

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

alex-spies added 2 commits January 8, 2024 16:22

Merge remote-tracking branch 'upstream/main' into esql-fix-wrong-type…

5e421ea

…-in-lucene-filter

Fix tests

4149153

Make our linter happy

dd9d71b

costin reviewed Jan 24, 2024

View reviewed changes

alex-spies added 6 commits January 29, 2024 18:21

Merge remote-tracking branch 'upstream/main' into esql-fix-wrong-type…

e6edfa2

…-in-lucene-filter

Move new tranlator into EsqlQueryTranslators

e57d16a

Revert QL ExpressionTranslators

2c8d04c

Explicitly pass null in LoggerMessageFormat.format

4cb572e

Walk object tree instead of string comparisons

f08b231

Use stream with try-with-resources

41fa7a8

alex-spies requested a review from costin January 29, 2024 18:12

astefan approved these changes Jan 29, 2024

View reviewed changes

costin reviewed Jan 30, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into esql-fix-wrong-type…

2e227d2

…-in-lucene-filter

costin approved these changes Jan 30, 2024

View reviewed changes

alex-spies added 3 commits January 30, 2024 17:34

Clean RestEsqlTestCase

6176da5

Clean LocalPhysicalPlanOptimizerTests

f05a41f

Clean EsqlTranslatorHandler

e6cba69

alex-spies requested a review from a team as a code owner January 30, 2024 16:55

astefan reviewed Jan 30, 2024

View reviewed changes

...ingle-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/RestEsqlIT.java Outdated Show resolved Hide resolved

Remove leftover

766435a

breskeby approved these changes Jan 30, 2024

View reviewed changes

alex-spies merged commit 1da5f99 into elastic:main Jan 31, 2024
15 checks passed

alex-spies deleted the esql-fix-wrong-type-in-lucene-filter branch January 31, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Correct out-of-range filter pushdowns #99961

ESQL: Correct out-of-range filter pushdowns #99961

alex-spies commented Sep 27, 2023 •

edited

Loading

alex-spies Oct 2, 2023

elasticsearchmachine commented Oct 2, 2023

elasticsearchmachine commented Oct 2, 2023

elasticsearchmachine commented Oct 2, 2023

costin left a comment

alex-spies commented Oct 4, 2023

elasticsearchmachine commented Jan 2, 2024

elasticsearchmachine commented Jan 23, 2024

alex-spies commented Jan 23, 2024

costin Jan 24, 2024

alex-spies Jan 29, 2024

costin left a comment

costin Jan 24, 2024

alex-spies Jan 29, 2024

astefan left a comment

costin left a comment

costin Jan 30, 2024

alex-spies Jan 30, 2024

costin Jan 30, 2024

alex-spies Jan 30, 2024 •

edited

Loading

costin Jan 30, 2024

alex-spies Jan 30, 2024

alex-spies Jan 30, 2024

alex-spies commented Jan 31, 2024

alex-spies commented Jan 31, 2024

ESQL: Correct out-of-range filter pushdowns #99961

ESQL: Correct out-of-range filter pushdowns #99961

Conversation

alex-spies commented Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 2, 2023

elasticsearchmachine commented Oct 2, 2023

elasticsearchmachine commented Oct 2, 2023

costin left a comment

Choose a reason for hiding this comment

alex-spies commented Oct 4, 2023

elasticsearchmachine commented Jan 2, 2024

elasticsearchmachine commented Jan 23, 2024

alex-spies commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies commented Jan 31, 2024

alex-spies commented Jan 31, 2024

alex-spies commented Sep 27, 2023 •

edited

Loading

alex-spies Jan 30, 2024 •

edited

Loading