ESQL: Fix null comparison type checking by kanoshiou · Pull Request #140660 · elastic/elasticsearch

kanoshiou · 2026-01-14T15:04:41Z

This PR fixes an issue where comparing null with other types incorrectly produced a type incompatibility error.

For example, ROW x = null, y = 1 | WHERE x == y would fail with a type error instead of correctly evaluating to null. Null should be compatible with any type and evaluate to null at runtime, matching the behavior of EsqlBinaryComparison.checkCompatibility.

Fixes #140460

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

elasticsearchmachine · 2026-01-22T07:58:16Z

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

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

astefan · 2026-02-10T13:05:41Z

buildkite test this

astefan

I think the fix is ok, but it needs more refining before merge:

in In:242 the comment is no longer accurate. That specific method has been removed.
check any csv-spec tests that we have for unsigned_long, null and in. If we don't have any, please add some. I would start with unsigned_long.csv-spec.
same ask as the previous one, but for tests that use null in boolean logical operations, for example from ul_logs | eval x = null OR bytes_in > null or from ul_logs | eval x = null or bytes_in > 2::unsigned_long

astefan · 2026-02-10T15:13:50Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

@@ -241,12 +242,10 @@ private static List<BiConsumer<LogicalPlan, Failures>> planCheckers(LogicalPlan
    private static void checkOperationsOnUnsignedLong(LogicalPlan p, Failures failures) {


I suggest the following change to this method (simplified and with a better comment):

/** * This validates only the negation operator, the rest of the validation is performed in * {@link org.elasticsearch.xpack.esql.analysis.Verifier#validateBinaryComparison(BinaryComparison)} * and * {@link org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.EsqlBinaryComparison#areTypesCompatible(DataType, DataType)} */ private static void checkOperationsOnUnsignedLong(LogicalPlan p, Failures failures) { p.forEachExpression(Neg.class, neg -> { Failure f = validateUnsignedLongNegation(neg); if (f != null) { failures.add(f); } }); }

astefan · 2026-02-10T15:20:26Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

+     *
+     * @return a Failure if the left operand type is not allowed, null otherwise
+     */
+    private static Failure checkBinaryComparisonTypes(BinaryComparison bc) {


Change the name of this method accordingly, since now it looks only at the left side of a binary comparison.

astefan · 2026-02-10T15:22:05Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

-        return null;
-    }
-
-    /** Ensure that UNSIGNED_LONG types are not implicitly converted when used in arithmetic binary operator, as this cannot be done since:


Save this comment somewhere, since it's valuable information regarding unsigned_long types compatibility.

astefan · 2026-02-10T15:24:12Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

+     * Test that null comparisons are valid and don't produce type incompatibility errors.
+     * Null should be compatible with any type in binary comparisons.
+     */
+    public void testNullComparisonValidation() {


Consider adding tests comparing unsigned_long with null as well.

astefan · 2026-02-10T15:27:35Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/null.csv-spec

 10022          | Shahaf             | Famili
 ;
+
+###############################################


Add more complex tests that get a null from an operation for example.
row x = null, y = 1, z = 2 | eval t = x + 1 > z

astefan · 2026-02-10T15:30:28Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

-
-        assertEquals(
-            "1:19: first argument of [emp_no == ?] is [numeric] so second argument must also be [numeric] but was [null]",
-            error("from test | where emp_no == ?", new Object[] { null })


Don't remove this query entirely, but move it to the method above and validate that a param null is still being passed correctly.

kanoshiou · 2026-02-12T15:11:52Z

Thank you for your review, @astefan!

I’ve updated the code based on your comments. I also revised the areCompatible logic in both In and DataType. If I’ve missed anything, please let me know and I’ll address it promptly.

astefan · 2026-02-13T16:25:26Z

buildkite test this

kanoshiou · 2026-02-15T06:46:33Z

Hi @astefan, I believe the failing test is not caused by this PR. It appears to be an issue specific to the CI runner.

astefan · 2026-02-16T07:22:12Z

buildkite test this

kanoshiou · 2026-02-16T09:16:14Z

Hi @astefan,

I've added the warnings for unsigned_long.UnsignedLongInWithNull. I wasn't able to reproduce the failures for the other two tests, but it looks like they are tracked separately in issue #138231.

astefan · 2026-02-16T09:18:50Z

buildkite test this

kanoshiou · 2026-02-16T11:21:01Z

This is quite unexpected. I have switched to regex warnings and double-checked the tests to ensure they pass locally.

astefan · 2026-02-17T09:36:19Z

buildkite test this

Remove redundant comments

…null-comparison-type-check

kanoshiou · 2026-02-17T12:15:56Z

Thanks for the patches, @astefan.

Actually, this removal doesn't address these failures. ":x-pack:plugin:esql:test" --tests "org.elasticsearch.xpack.esql.CsvTests" -Dtests.method="test {csv-spec:unsigned_long.UnsignedLongInWithNull}" would still fail. Issue #139262 is currently tracking the "missing warning" problem.

I previously ran EsqlSpecIT but skipped CsvTests, and now I believe adding the necessary regex escape characters (\) should resolve the current failures.

But I am not quite sure if AssertWarnings.AllowedRegexes#assertWarnings is doing the right job because it is skipped if there are no warnings.

astefan · 2026-02-17T12:37:26Z

buildkite test this

astefan · 2026-02-17T15:55:56Z

Thank you for explaining @kanoshiou. #139262 cannot be easily reproduced, actually at all so far. Those failures have been noticed in several runs in CI, but not at all locally in tests.

I have tested that query with no warnings in csv-spec file locally and both these runs were ok with it:

gradlew :x-pack:plugin:esql:qa:server:single-node:javaRestTest --tests "org.elasticsearch.xpack.esql.qa.single_node.EsqlSpecIT" -Dtests.method="*unsigned_long*"
gradlew :x-pack:plugin:esql:test -Dtests.class=org.elasticsearch.xpack.esql.CsvTests -Dtests.method="*unsigned_long*"

But, I think there is an explanation and for this test to be future-safe (multiple CI runs and highly randomized environments) I think we need to make this query (maybe others, haven't looked into it) more deterministic. Take a look at this javadoc https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java#L184-L194 and the one from SingleValueMatchQuery. This means that depending on how the Lucene index is accessed, a query that hits the index might or might not issue a Warning. On the other hand, with CsvTests this will allways happen (emitting warnings).

For that specific query I suggest a change like the following, if I am not missing anything: FROM ul_logs | WHERE id IN (42, 85) | LIMIT 5 | SORT id | WHERE bytes_in IN (to_ul(0), null) | KEEP id, bytes_in (those two IDs are the single ones from that index that have multi-values). Let me know what you think.

kanoshiou · 2026-02-17T16:59:41Z

Thanks for the guidance, @astefan.

The Javadocs were really helpful and cleared up my confusion about the inconsistent test results. I’ve updated the test to match your suggestion. Since there appear to be other non-deterministic tests as well, I’d be happy to address those in a separate PR if you'd like.

astefan · 2026-02-18T08:59:14Z

buildkite test this

astefan

LGTM

astefan · 2026-02-13T16:29:54Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/unsigned_long.csv-spec

+;
+
+
+# unsigned_long field not equal to null with WHERE (null rows filtered out)


This filters out all rows not only "null rows".

(cherry picked from commit b30aa50)

(cherry picked from commit b30aa50) Co-authored-by: kanoshiou <uiaao@tuta.io>

Fix ESQL null comparison type checking

c884888

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.4.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jan 14, 2026

kanoshiou changed the title ~~ESQL: Fix ESQL null comparison type checking~~ ESQL: Fix null comparison type checking Jan 14, 2026

kanoshiou and others added 2 commits January 14, 2026 23:07

Update 140660

91a9383

Merge branch 'main' into null-comparison-type-check

dbd2f5e

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

gareth-ellis added >bug :Analytics/ES|QL AKA ESQL labels Jan 22, 2026

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 22, 2026

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jan 22, 2026

Merge branch 'main' into null-comparison-type-check

3d39d3c

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

astefan self-assigned this Feb 10, 2026

astefan self-requested a review February 10, 2026 10:22

astefan reviewed Feb 10, 2026

View reviewed changes

kanoshiou added 3 commits February 12, 2026 20:57

Merge branch 'main' into null-comparison-type-check

f1408a3

Update tests

c2a6e4e

Update null comparison in IN

2e8c3b2

kanoshiou added 2 commits February 12, 2026 23:12

Merge branch 'main' into null-comparison-type-check

945a925

Replace if with switch

73788f6

Merge branch 'main' into null-comparison-type-check

7969efc

Add warnings

7d98cfe

kanoshiou added 2 commits February 16, 2026 19:16

Switch to regex warnings

b5ccdc8

Merge branch 'main' into null-comparison-type-check

d086bcb

astefan added 2 commits February 17, 2026 13:50

Fix test by removing the unnecessary warnings check

98e51d0

Remove redundant comments

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

b4ce19c

…null-comparison-type-check

warningRegex

8228795

kanoshiou added 2 commits February 18, 2026 00:58

Use multi-valued docs

8f35659

Merge branch 'main' into null-comparison-type-check

6fe8e0c

Merge branch 'main' into null-comparison-type-check

a481249

astefan approved these changes Feb 18, 2026

View reviewed changes

astefan merged commit b30aa50 into elastic:main Feb 18, 2026
38 checks passed

astefan pushed a commit to astefan/elasticsearch that referenced this pull request Mar 17, 2026

ESQL: Fix null comparison type checking (elastic#140660)

639d0f4

(cherry picked from commit b30aa50)

astefan mentioned this pull request Mar 17, 2026

ESQL: Fix null comparison type checking (#140660) #144410

Merged

astefan added a commit that referenced this pull request Mar 17, 2026

ESQL: Fix null comparison type checking (#140660) (#144410)

67bc79b

(cherry picked from commit b30aa50) Co-authored-by: kanoshiou <uiaao@tuta.io>

astefan added the v9.3.3 label Mar 17, 2026

		@@ -241,12 +242,10 @@ private static List<BiConsumer<LogicalPlan, Failures>> planCheckers(LogicalPlan
		private static void checkOperationsOnUnsignedLong(LogicalPlan p, Failures failures) {

		;


		# unsigned_long field not equal to null with WHERE (null rows filtered out)

Conversation

kanoshiou commented Jan 14, 2026

Uh oh!

elasticsearchmachine commented Jan 22, 2026

Uh oh!

astefan commented Feb 10, 2026

Uh oh!

astefan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

kanoshiou commented Feb 12, 2026

Uh oh!

astefan commented Feb 13, 2026

Uh oh!

kanoshiou commented Feb 15, 2026

Uh oh!

astefan commented Feb 16, 2026

Uh oh!

kanoshiou commented Feb 16, 2026

Uh oh!

astefan commented Feb 16, 2026

Uh oh!

kanoshiou commented Feb 16, 2026

Uh oh!

astefan commented Feb 17, 2026

Uh oh!

kanoshiou commented Feb 17, 2026

Uh oh!

astefan commented Feb 17, 2026

Uh oh!

astefan commented Feb 17, 2026

Uh oh!

kanoshiou commented Feb 17, 2026

Uh oh!

astefan commented Feb 18, 2026

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

astefan Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astefan left a comment •

edited

Loading