Implement review suggestions

Author: luigidellaquila

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

@@ -58,12 +58,11 @@ public Literal(Source source, Object value, DataType dataType) {
 
     private boolean noPlainStrings(Object value, DataType dataType) {
         if (dataType == KEYWORD || dataType == TEXT || dataType == VERSION) {
-            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;
+            };
         }
         return true;
     }
@@ -146,6 +145,7 @@ public String toString() {
             str = BytesRefs.toString(value);
         } else if (dataType == VERSION && value instanceof BytesRef br) {
             str = new Version(br).toString();
+            // TODO review how we manage IPs: https://github.com/elastic/elasticsearch/issues/129605
             // } else if (dataType == IP && value instanceof BytesRef ip) {
             // str = DocValueFormat.IP.format(ip);
         } else {
@@ -186,6 +186,10 @@ public static Literal of(Expression source, Object value) {
         return new Literal(source.source(), value, source.dataType());
     }
 
+    public static Literal keyword(Source source, String literal) {
+        return new Literal(source, BytesRefs.toBytesRef(literal), KEYWORD);
+    }
+
     /**
      * Not all literal values are currently supported in StreamInput/StreamOutput as generic values.
      * This mapper allows for addition of new and interesting values without (yet) adding to StreamInput/Output.

x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/function/scalar/FunctionTestUtils.java

@@ -20,7 +20,7 @@ public static Literal l(Object value) {
     }
 
     public static Literal l(Object value, DataType type) {
-        if (value instanceof String && (type == DataType.TEXT || type == DataType.KEYWORD)) {
+        if ((type == DataType.TEXT || type == DataType.KEYWORD) && value instanceof String) {
             value = BytesRefs.toBytesRef(value);
         }
         return new Literal(EMPTY, value, type);

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

@@ -43,7 +43,7 @@ public static Literal of(Source source, Object value) {
             return (Literal) value;
         }
         DataType type = DataType.fromJava(value);
-        if (value instanceof String && (type == TEXT || type == KEYWORD)) {
+        if ((type == TEXT || type == KEYWORD) && value instanceof String) {
             value = BytesRefs.toBytesRef(value);
         }
         return new Literal(source, value, DataType.fromJava(value));

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

@@ -398,7 +398,7 @@ eth0           |gamma          |fe80::cae2:65ff:fece:feb9
 lo0            |gamma          |fe80::cae2:65ff:fece:feb9                                                         |fe81::cae2:65ff:fece:feb9
 ;
 
-cicdirMatchEqualsInsOrsIPs
+cidrMatchEqualsInsOrsIPs
 required_capability: combine_disjunctive_cidrmatches
 
 FROM hosts

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

@@ -7,7 +7,6 @@
 
 package org.elasticsearch.xpack.esql.analysis;
 
-import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.logging.HeaderWarning;
 import org.elasticsearch.common.logging.LoggerMessageFormat;
 import org.elasticsearch.common.lucene.BytesRefs;
@@ -320,7 +319,7 @@ protected LogicalPlan rule(Enrich plan, AnalyzerContext context) {
                 // the policy does not exist
                 return plan;
             }
-            final String policyName = ((BytesRef) plan.policyName().fold(FoldContext.small() /* TODO remove me */)).utf8ToString();
+            final String policyName = BytesRefs.toString(plan.policyName().fold(FoldContext.small() /* TODO remove me */));
             final var resolved = context.enrichResolution().getResolvedPolicy(policyName, plan.mode());
             if (resolved != null) {
                 var policy = new EnrichPolicy(resolved.matchType(), null, List.of(), resolved.matchField(), resolved.enrichFields());
@@ -1579,7 +1578,7 @@ private static UnresolvedAttribute unresolvedAttribute(Expression value, String
         private static Expression castStringLiteralToTemporalAmount(Expression from) {
             try {
                 TemporalAmount result = maybeParseTemporalAmount(
-                    ((BytesRef) from.fold(FoldContext.small() /* TODO remove me */)).utf8ToString().strip()
+                    BytesRefs.toString(from.fold(FoldContext.small() /* TODO remove me */)).strip()
                 );
                 if (result == null) {
                     return from;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Count.java

@@ -9,7 +9,6 @@
 
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.compute.aggregation.AggregatorFunctionSupplier;
 import org.elasticsearch.compute.aggregation.CountAggregatorFunction;
 import org.elasticsearch.compute.data.AggregateMetricDoubleBlockBuilder;
@@ -164,7 +163,7 @@ public Expression surrogate() {
             return new Mul(
                 s,
                 new Coalesce(s, new MvCount(s, field), List.of(new Literal(s, 0, DataType.INTEGER))),
-                new Count(s, new Literal(s, BytesRefs.toBytesRef(StringUtils.WILDCARD), DataType.KEYWORD))
+                new Count(s, Literal.keyword(s, StringUtils.WILDCARD))
             );
         }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java

@@ -8,7 +8,6 @@
 
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.compute.aggregation.AggregatorFunctionSupplier;
 import org.elasticsearch.compute.aggregation.SumDoubleAggregatorFunctionSupplier;
 import org.elasticsearch.compute.aggregation.SumIntAggregatorFunctionSupplier;
@@ -144,8 +143,6 @@ public Expression surrogate() {
         }
 
         // SUM(const) is equivalent to MV_SUM(const)*COUNT(*).
-        return field.foldable()
-            ? new Mul(s, new MvSum(s, field), new Count(s, new Literal(s, BytesRefs.toBytesRef(StringUtils.WILDCARD), DataType.KEYWORD)))
-            : null;
+        return field.foldable() ? new Mul(s, new MvSum(s, field), new Count(s, Literal.keyword(s, StringUtils.WILDCARD))) : null;
     }
 }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

@@ -11,6 +11,7 @@
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.common.time.DateUtils;
 import org.elasticsearch.compute.ann.Evaluator;
 import org.elasticsearch.compute.ann.Fixed;
@@ -317,7 +318,7 @@ private ExpressionEvaluator.Factory toEvaluator(
 
         if (unit.foldable()) {
             try {
-                Part datePartField = Part.resolve(((BytesRef) unit.fold(toEvaluator.foldCtx())).utf8ToString());
+                Part datePartField = Part.resolve(BytesRefs.toString(unit.fold(toEvaluator.foldCtx())));
                 return constantFactory.build(source(), datePartField, startTimestampEvaluator, endTimestampEvaluator);
             } catch (IllegalArgumentException e) {
                 throw new InvalidArgumentException("invalid unit format for [{}]: {}", sourceText(), e.getMessage());

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java

@@ -65,8 +65,8 @@ public class MvSort extends EsqlScalarFunction implements OptionalArgument, Post
 
     private final Expression field, order;
 
-    private static final Literal ASC = new Literal(Source.EMPTY, BytesRefs.toBytesRef("ASC"), DataType.KEYWORD);
-    private static final Literal DESC = new Literal(Source.EMPTY, BytesRefs.toBytesRef("DESC"), DataType.KEYWORD);
+    private static final Literal ASC = Literal.keyword(Source.EMPTY, "ASC");
+    private static final Literal DESC = Literal.keyword(Source.EMPTY, "DESC");
 
     private static final String INVALID_ORDER_ERROR = "Invalid order value in [{}], expected one of [{}, {}] but got [{}]";
 
@@ -157,13 +157,12 @@ public EvalOperator.ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvalua
                     sourceText(),
                     BytesRefs.toString(ASC.value()),
                     BytesRefs.toString(DESC.value()),
-                    ((BytesRef) order.fold(toEvaluator.foldCtx())).utf8ToString()
+                    BytesRefs.toString(order.fold(toEvaluator.foldCtx()))
                 )
             );
         }
         if (order != null && order.foldable()) {
-            ordering = ((BytesRef) order.fold(toEvaluator.foldCtx())).utf8ToString()
-                .equalsIgnoreCase(((BytesRef) ASC.value()).utf8ToString());
+            ordering = BytesRefs.toString(order.fold(toEvaluator.foldCtx())).equalsIgnoreCase(BytesRefs.toString(ASC.value()));
         }
 
         return switch (PlannerUtils.toElementType(field.dataType())) {
@@ -245,9 +244,9 @@ public void postOptimizationVerification(Failures failures) {
                     order,
                     INVALID_ORDER_ERROR,
                     sourceText(),
-                    ((BytesRef) ASC.value()).utf8ToString(),
-                    ((BytesRef) DESC.value()).utf8ToString(),
-                    ((BytesRef) order.fold(FoldContext.small() /* TODO remove me */)).utf8ToString()
+                    BytesRefs.toString(ASC.value()),
+                    BytesRefs.toString(DESC.value()),
+                    BytesRefs.toString(order.fold(FoldContext.small() /* TODO remove me */))
                 )
             );
         }
@@ -264,8 +263,8 @@ private boolean isValidOrder() {
                 o = os;
             }
             if (o == null
-                || o.equalsIgnoreCase(((BytesRef) ASC.value()).utf8ToString()) == false
-                    && o.equalsIgnoreCase(((BytesRef) DESC.value()).utf8ToString()) == false) {
+                || o.equalsIgnoreCase(BytesRefs.toString(ASC.value())) == false
+                    && o.equalsIgnoreCase(BytesRefs.toString(DESC.value())) == false) {
                 isValidOrder = false;
             }
         }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Replace.java

@@ -12,6 +12,7 @@
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.compute.ann.Evaluator;
 import org.elasticsearch.compute.ann.Fixed;
 import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;
@@ -199,7 +200,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
         if (regex.foldable() && regex.dataType() == DataType.KEYWORD) {
             Pattern regexPattern;
             try {
-                regexPattern = Pattern.compile(((BytesRef) regex.fold(toEvaluator.foldCtx())).utf8ToString());
+                regexPattern = Pattern.compile(BytesRefs.toString(regex.fold(toEvaluator.foldCtx())));
             } catch (PatternSyntaxException pse) {
                 // TODO this is not right (inconsistent). See also https://github.com/elastic/elasticsearch/issues/100038
                 // this should generate a header warning and return null (as do the rest of this functionality in evaluators),