-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL: No plain strings in Literal #129399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
af84776
e6f4383
48ea6e0
a7b2928
a3ccc10
79b3550
81194d1
b726351
c7cd50a
6e9475e
e56aab3
094c98f
c301a3b
87796c4
da3f737
67ff8e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,18 +11,24 @@ | |
| 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.xpack.esql.core.QlIllegalArgumentException; | ||
| import org.elasticsearch.xpack.esql.core.tree.NodeInfo; | ||
| import org.elasticsearch.xpack.esql.core.tree.Source; | ||
| import org.elasticsearch.xpack.esql.core.type.DataType; | ||
| import org.elasticsearch.xpack.esql.core.util.PlanStreamInput; | ||
| import org.elasticsearch.xpack.versionfield.Version; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| import static org.elasticsearch.xpack.esql.core.type.DataType.CARTESIAN_POINT; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION; | ||
| import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.CARTESIAN; | ||
| import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO; | ||
|
|
||
|
|
@@ -45,10 +51,25 @@ public class Literal extends LeafExpression { | |
|
|
||
| public Literal(Source source, Object value, DataType dataType) { | ||
| super(source); | ||
| assert noPlainStrings(value, dataType); | ||
| this.dataType = dataType; | ||
| this.value = value; | ||
| } | ||
|
|
||
| private boolean noPlainStrings(Object value, DataType dataType) { | ||
| if (dataType == KEYWORD || dataType == TEXT || dataType == VERSION) { | ||
| if (value == null) { | ||
| return true; | ||
| } | ||
| return switch (value) { | ||
| case String s -> false; | ||
| case Collection<?> c -> c.stream().allMatch(x -> noPlainStrings(x, dataType)); | ||
| default -> true; | ||
| }; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| private static Literal readFrom(StreamInput in) throws IOException { | ||
| Source source = Source.readFrom((StreamInput & PlanStreamInput) in); | ||
| Object value = in.readGenericValue(); | ||
|
|
@@ -122,7 +143,21 @@ public boolean equals(Object obj) { | |
|
|
||
| @Override | ||
| public String toString() { | ||
| String str = String.valueOf(value); | ||
| String str; | ||
| if (dataType == KEYWORD || dataType == TEXT) { | ||
| 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); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just in case, is this intentionally commented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah... unfortunately it is, for two reasons:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add this as a comment/todo referencing an issue to resolve it eventually? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created an issue and I'm linking it in a TODO |
||
| } else { | ||
| str = String.valueOf(value); | ||
| } | ||
|
|
||
| if (str == null) { | ||
| str = "null"; | ||
| } | ||
| if (str.length() > 500) { | ||
| return str.substring(0, 500) + "..."; | ||
| } | ||
|
|
@@ -154,6 +189,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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an improvement? Or were Versions a readable String before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.