From 7045420ce7e425d25b9ba66778e0cd78a5f7e4e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Thu, 10 Jul 2025 17:41:05 +0200 Subject: [PATCH 1/2] ESQL: Fix LIMIT NPE with null value (#130914) Fixes https://github.com/elastic/elasticsearch/issues/130908 --- docs/changelog/130914.yaml | 6 +++ x-pack/plugin/build.gradle | 1 + .../xpack/esql/action/EsqlCapabilities.java | 5 +++ .../xpack/esql/parser/LogicalPlanBuilder.java | 27 +++++------ .../xpack/esql/analysis/ParsingTests.java | 45 ++++++++++++++++--- .../esql/parser/StatementParserTests.java | 4 -- .../rest-api-spec/test/esql/10_basic.yml | 4 +- 7 files changed, 68 insertions(+), 24 deletions(-) create mode 100644 docs/changelog/130914.yaml diff --git a/docs/changelog/130914.yaml b/docs/changelog/130914.yaml new file mode 100644 index 0000000000000..da38b52e5f879 --- /dev/null +++ b/docs/changelog/130914.yaml @@ -0,0 +1,6 @@ +pr: 130914 +summary: Fix LIMIT NPE with null value +area: ES|QL +type: bug +issues: + - 130908 diff --git a/x-pack/plugin/build.gradle b/x-pack/plugin/build.gradle index b69208f175201..fcae5576e57dc 100644 --- a/x-pack/plugin/build.gradle +++ b/x-pack/plugin/build.gradle @@ -230,6 +230,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure({ task -> task.skipTest("esql/191_lookup_join_on_datastreams/data streams not supported in LOOKUP JOIN", "Added support for aliases in JOINs") task.skipTest("esql/190_lookup_join/non-lookup index", "Error message changed") task.skipTest("esql/192_lookup_join_on_aliases/alias-pattern-multiple", "Error message changed") + task.skipTest("esql/10_basic/Test wrong LIMIT parameter", "Error message changed") }) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index f8443e21cab5f..f64988a332f70 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -956,6 +956,11 @@ public enum Cap { */ PARAMETER_FOR_LIMIT, + /** + * Changed and normalized the LIMIT error message. + */ + NORMALIZED_LIMIT_ERROR_MESSAGE, + /** * Enable support for index aliases in lookup joins */ diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java index 13e84061d843d..94fcbd1923cc2 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java @@ -376,21 +376,22 @@ public PlanFactory visitWhereCommand(EsqlBaseParser.WhereCommandContext ctx) { public PlanFactory visitLimitCommand(EsqlBaseParser.LimitCommandContext ctx) { Source source = source(ctx); Object val = expression(ctx.constant()).fold(FoldContext.small() /* TODO remove me */); - if (val instanceof Integer i) { - if (i < 0) { - throw new ParsingException(source, "Invalid value for LIMIT [" + i + "], expecting a non negative integer"); - } + if (val instanceof Integer i && i >= 0) { return input -> new Limit(source, new Literal(source, i, DataType.INTEGER), input); - } else { - throw new ParsingException( - source, - "Invalid value for LIMIT [" - + BytesRefs.toString(val) - + ": " - + (expression(ctx.constant()).dataType() == KEYWORD ? "String" : val.getClass().getSimpleName()) - + "], expecting a non negative integer" - ); } + + String valueType = expression(ctx.constant()).dataType().typeName(); + + throw new ParsingException( + source, + "value of [" + + source.text() + + "] must be a non negative integer, found value [" + + ctx.constant().getText() + + "] type [" + + valueType + + "]" + ); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java index 5cfa311ec4f55..f598b8e943ea1 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java @@ -19,7 +19,10 @@ import org.elasticsearch.xpack.esql.index.EsIndex; import org.elasticsearch.xpack.esql.index.IndexResolution; import org.elasticsearch.xpack.esql.parser.EsqlParser; +import org.elasticsearch.xpack.esql.parser.ParserUtils; import org.elasticsearch.xpack.esql.parser.ParsingException; +import org.elasticsearch.xpack.esql.parser.QueryParam; +import org.elasticsearch.xpack.esql.parser.QueryParams; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.esql.plan.logical.Row; import org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter; @@ -153,9 +156,34 @@ public void testJoinTwiceOnTheSameField_TwoLookups() { } public void testInvalidLimit() { - assertEquals("1:13: Invalid value for LIMIT [foo: String], expecting a non negative integer", error("row a = 1 | limit \"foo\"")); - assertEquals("1:13: Invalid value for LIMIT [1.2: Double], expecting a non negative integer", error("row a = 1 | limit 1.2")); - assertEquals("1:13: Invalid value for LIMIT [-1], expecting a non negative integer", error("row a = 1 | limit -1")); + assertLimitWithAndWithoutParams("foo", "\"foo\"", DataType.KEYWORD); + assertLimitWithAndWithoutParams(1.2, "1.2", DataType.DOUBLE); + assertLimitWithAndWithoutParams(-1, "-1", DataType.INTEGER); + assertLimitWithAndWithoutParams(true, "true", DataType.BOOLEAN); + assertLimitWithAndWithoutParams(false, "false", DataType.BOOLEAN); + assertLimitWithAndWithoutParams(null, "null", DataType.NULL); + } + + private void assertLimitWithAndWithoutParams(Object value, String valueText, DataType type) { + assertEquals( + "1:13: value of [limit " + + valueText + + "] must be a non negative integer, found value [" + + valueText + + "] type [" + + type.typeName() + + "]", + error("row a = 1 | limit " + valueText) + ); + + assertEquals( + "1:13: value of [limit ?param] must be a non negative integer, found value [?param] type [" + type.typeName() + "]", + error( + "row a = 1 | limit ?param", + new QueryParams(List.of(new QueryParam("param", value, type, ParserUtils.ParamClassification.VALUE))) + ) + ); + } public void testInvalidSample() { @@ -177,13 +205,20 @@ public void testInvalidSample() { ); } - private String error(String query) { - ParsingException e = expectThrows(ParsingException.class, () -> defaultAnalyzer.analyze(parser.createStatement(query))); + private String error(String query, QueryParams params) { + ParsingException e = expectThrows( + ParsingException.class, + () -> defaultAnalyzer.analyze(parser.createStatement(query, params)) + ); String message = e.getMessage(); assertTrue(message.startsWith("line ")); return message.substring("line ".length()); } + private String error(String query) { + return error(query, new QueryParams()); + } + private static IndexResolution loadIndexResolution(String name) { return IndexResolution.valid(new EsIndex(INDEX_NAME, LoadMapping.loadMapping(name))); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java index 3893705da5014..e5aebc7281e37 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java @@ -986,10 +986,6 @@ public void testBasicLimitCommand() { assertThat(limit.children().get(0).children().get(0), instanceOf(UnresolvedRelation.class)); } - public void testLimitConstraints() { - expectError("from text | limit -1", "line 1:13: Invalid value for LIMIT [-1], expecting a non negative integer"); - } - public void testBasicSortCommand() { LogicalPlan plan = statement("from text | where true | sort a+b asc nulls first, x desc nulls last | sort y asc | sort z desc"); assertThat(plan, instanceOf(OrderBy.class)); diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml index 5b0492f9e847e..b9d20d4cd40cf 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/10_basic.yml @@ -555,11 +555,11 @@ FROM EVAL SORT LIMIT with documents_found: - method: POST path: /_query parameters: [ ] - capabilities: [ parameter_for_limit ] + capabilities: [ normalized_limit_error_message ] reason: "named or positional parameters for field names" - do: - catch: "/Invalid value for LIMIT \\[foo: String\\], expecting a non negative integer/" + catch: "/value of \\[limit \\?l\\] must be a non negative integer, found value \\[\\?l\\] type \\[keyword\\]/" esql.query: body: query: 'from test | limit ?l' From 1ffea8d1dd67e0072d3a002f3404a91a8ac3b33c Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 10 Jul 2025 16:12:58 +0000 Subject: [PATCH 2/2] [CI] Auto commit changes from spotless --- .../org/elasticsearch/xpack/esql/analysis/ParsingTests.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java index f598b8e943ea1..b0f27d8b21418 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java @@ -206,10 +206,7 @@ public void testInvalidSample() { } private String error(String query, QueryParams params) { - ParsingException e = expectThrows( - ParsingException.class, - () -> defaultAnalyzer.analyze(parser.createStatement(query, params)) - ); + ParsingException e = expectThrows(ParsingException.class, () -> defaultAnalyzer.analyze(parser.createStatement(query, params))); String message = e.getMessage(); assertTrue(message.startsWith("line ")); return message.substring("line ".length());