ES|QL: No plain strings in Literal by luigidellaquila · Pull Request #129399 · elastic/elasticsearch

luigidellaquila · 2025-06-13T09:46:32Z

Making sure that all the TEXT, KEYWORD and VERSION Literals contain a BytesRef (and not a plain java.lang.String).

The mix of the two was the reason for some ClassCastExceptions, eg. #129322

Steps:

Force the conversion from String to BytesRef inside Literal constructor
Make all the tests pass
Replace the forced conversion with an assert, checking that no java.lang.String is passed to the constructor
Make tests pass again

elasticsearchmachine · 2025-06-17T14:01:58Z

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

idegtiarenko · 2025-06-17T14:03:59Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java

+        } else if (dataType == VERSION && value instanceof BytesRef br) {
+            str = new Version(br).toString();
+            // } else if (dataType == IP && value instanceof BytesRef ip) {
+            // str = DocValueFormat.IP.format(ip);


Just in case, is this intentionally commented?

Yeah... unfortunately it is, for two reasons:

IP conversion from/to BytesRefs could trigger assertion errors, and it's pretty annoying in tests

We are explicitly using strings for IPs in some cases, see CobineDisjunctions; we'll have to change that before we can force IPs as BytesRefs

Could you please add this as a comment/todo referencing an issue to resolve it eventually?

I created an issue and I'm linking it in a TODO

ivancea

LGTM!

ivancea · 2025-06-17T14:30:29Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java

+        if (dataType == KEYWORD || dataType == TEXT) {
+            str = BytesRefs.toString(value);
+        } else if (dataType == VERSION && value instanceof BytesRef br) {
+            str = new Version(br).toString();


Is this an improvement? Or were Versions a readable String before?

We had some cases (mostly unit tests) where we passed plain strings to VERSION literals.
Even for tests it's not a good thing, as we are testing something that is different than the actual production execution.

ivancea · 2025-06-17T14:32:57Z

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

 ;

-cdirMatchEqualsInsOrsIPs
+cicdirMatchEqualsInsOrsIPs


Suggested change

cicdirMatchEqualsInsOrsIPs

cidrMatchEqualsInsOrsIPs

ivancea · 2025-06-17T14:38:25Z

...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java

Nit: I would replace every cast+utf8ToString() to BytesRefs.toString(), if possible

ivancea · 2025-06-17T14:50:21Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToIpTests.java

    }

    private static String stringEvaluator(ToIp.LeadingZeros leadingZeros) {
+        if (leadingZeros == null) {


As this was here before, and in jdk21 the case null works (right?), do we need this change?

My IntelliJ (Java 21) keeps complaining, but maybe it's some wrong setting on my side

idegtiarenko · 2025-06-18T07:43:13Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java

+            if (value instanceof String) {
+                return false;
+            }
+            if (value instanceof Collection<?> c) {
+                return c.stream().allMatch(x -> noPlainStrings(x, dataType));
+            }


Nit: consider pattern matching here

Suggested change

if (value instanceof String) {

return false;

}

if (value instanceof Collection<?> c) {

return c.stream().allMatch(x -> noPlainStrings(x, dataType));

}

return switch (value) {

case String s -> false;

case Collection<?> c -> c.stream().allMatch(x -> noPlainStrings(x, dataType));

default -> true;

}

idegtiarenko · 2025-06-18T07:46:03Z

...est/java/org/elasticsearch/xpack/esql/core/expression/function/scalar/FunctionTestUtils.java

    }

    public static Literal l(Object value, DataType type) {
+        if (value instanceof String && (type == DataType.TEXT || type == DataType.KEYWORD)) {


NIT: type == DataType.TEXT || type == DataType.KEYWORD theck looks a bit cheaper. Do you mind making it first (before instanceof)?

idegtiarenko · 2025-06-18T07:46:15Z

x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/util/TestUtils.java

            return (Literal) value;
        }
+        DataType type = DataType.fromJava(value);
+        if (value instanceof String && (type == TEXT || type == KEYWORD)) {


idegtiarenko · 2025-06-18T07:52:34Z

...esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Space.java

            if (folded instanceof Integer num) {
                checkNumber(num);
-                return toEvaluator.apply(new Literal(source(), " ".repeat(num), KEYWORD));
+                return toEvaluator.apply(new Literal(source(), BytesRefs.toBytesRef(" ".repeat(num)), KEYWORD));


This seems to repeat a lot, I wonder if it is worth introducing:

pusblic static Literal.keyword(Source source, String literal) { return new Literal(source, BytesRefs.toBytesRef(literal), KEYWORD); }

to simplify construction of string literals?

👍 I think it makes sense

idegtiarenko

👍

luigidellaquila · 2025-06-18T09:19:38Z

Thanks for the feedback folks!

elasticsearchmachine · 2025-06-18T10:49:30Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 129399

luigidellaquila · 2025-06-19T10:50:13Z

Manual backport #129702

No plain strings in Literal

af84776

luigidellaquila added the :Analytics/ES|QL AKA ESQL label Jun 13, 2025

elasticsearchmachine added the v9.1.0 label Jun 13, 2025

luigidellaquila added 4 commits June 13, 2025 12:30

Fix enrich

e6f4383

Only pass strings as BytesRefs

48ea6e0

Fix

a7b2928

Merge branch 'main' into no_plain_strings_in_literals

a3ccc10

luigidellaquila added the >refactoring label Jun 13, 2025

luigidellaquila added 7 commits June 17, 2025 09:22

Merge branch 'main' into no_plain_strings_in_literals

79b3550

fix

81194d1

Fix unintended change

b726351

Fix error message

c7cd50a

Fix test

6e9475e

Merge branch 'main' into no_plain_strings_in_literals

e56aab3

More cleanup, remove rule

094c98f

luigidellaquila marked this pull request as ready for review June 17, 2025 14:01

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 17, 2025

idegtiarenko reviewed Jun 17, 2025

View reviewed changes

ivancea approved these changes Jun 17, 2025

View reviewed changes

luigidellaquila added v8.19.0 auto-backport Automatically create backport pull requests when merged labels Jun 18, 2025

idegtiarenko reviewed Jun 18, 2025

View reviewed changes

idegtiarenko approved these changes Jun 18, 2025

View reviewed changes

luigidellaquila added 3 commits June 18, 2025 10:28

Implement review suggestions

c301a3b

Merge branch 'main' into no_plain_strings_in_literals

87796c4

Fix

da3f737

Fix

67ff8e8

luigidellaquila enabled auto-merge (squash) June 18, 2025 09:20

luigidellaquila merged commit 22e592a into elastic:main Jun 18, 2025
27 checks passed

elasticsearchmachine added the backport pending label Jun 18, 2025

albertzaharovits pushed a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 18, 2025

ES|QL: No plain strings in Literal (elastic#129399)

73abc28

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Jun 19, 2025

ES|QL: No plain strings in Literal (elastic#129399)

ab4147a

luigidellaquila mentioned this pull request Jun 19, 2025

ES|QL: No plain strings in Literal (#129399) #129702

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jun 19, 2025

ES|QL: No plain strings in Literal (#129399) (#129702)

f413f28

kderusso pushed a commit to kderusso/elasticsearch that referenced this pull request Jun 23, 2025

ES|QL: No plain strings in Literal (elastic#129399)

64c172c

kanoshiou added a commit to kanoshiou/elasticsearch that referenced this pull request Jul 24, 2025

ES|QL: No plain strings in Literal elastic#129399

9f8bde5

Conversation

luigidellaquila commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko left a comment

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Jun 18, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 18, 2025

💔 Backport failed

Uh oh!

luigidellaquila commented Jun 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luigidellaquila commented Jun 13, 2025 •

edited

Loading