-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL - date nanos range bug? #125345
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
ESQL - date nanos range bug? #125345
Changes from 16 commits
f0dfea6
a934963
59da8f4
106d730
6892a01
97ccfe3
94a668b
b629514
5f4f830
004758c
50d9093
3807994
a91f2cc
212a4f5
3f40c8c
aa7c89b
056e106
3a032df
5da39d2
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pr: 125345 | ||
| summary: ESQL - date nanos range bug? | ||
| area: ES|QL | ||
| type: bug | ||
| issues: | ||
| - 125439 |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ | |
|
|
||
| import org.apache.lucene.util.BytesRef; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.logging.LogManager; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.xpack.esql.capabilities.TranslationAware; | ||
| import org.elasticsearch.xpack.esql.core.expression.Expression; | ||
| import org.elasticsearch.xpack.esql.core.expression.FoldContext; | ||
|
|
@@ -31,18 +33,23 @@ | |
|
|
||
| import static java.util.Arrays.asList; | ||
| import static org.elasticsearch.xpack.esql.core.expression.Foldables.valueOf; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_NANOS; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.IP; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION; | ||
| import static org.elasticsearch.xpack.esql.core.util.DateUtils.asDateTime; | ||
| import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.DEFAULT_DATE_NANOS_FORMATTER; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.DEFAULT_DATE_TIME_FORMATTER; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.dateTimeToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.ipToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.nanoTimeToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.versionToString; | ||
|
|
||
| // BETWEEN or range - is a mix of gt(e) AND lt(e) | ||
| public class Range extends ScalarFunction implements TranslationAware.SingleValueTranslationAware { | ||
|
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. There is a method |
||
| private static final Logger logger = LogManager.getLogger(Range.class); | ||
|
|
||
| private final Expression value, lower, upper; | ||
| private final boolean includeLower, includeUpper; | ||
|
|
@@ -210,12 +217,21 @@ private RangeQuery translate(TranslatorHandler handler) { | |
| String format = null; | ||
|
|
||
| DataType dataType = value.dataType(); | ||
| if (DataType.isDateTime(dataType) && DataType.isDateTime(lower.dataType()) && DataType.isDateTime(upper.dataType())) { | ||
| logger.trace( | ||
| "Translating Range into lucene query. dataType is [" + dataType + "] upper is [" + upper + "] lower is [" + lower + "]" | ||
|
||
| ); | ||
| if (dataType == DataType.DATETIME && lower.dataType() == DATETIME && upper.dataType() == DATETIME) { | ||
| l = dateTimeToString((Long) l); | ||
| u = dateTimeToString((Long) u); | ||
| format = DEFAULT_DATE_TIME_FORMATTER.pattern(); | ||
| } | ||
|
|
||
| if (dataType == DATE_NANOS && lower.dataType() == DATE_NANOS && upper.dataType() == DATE_NANOS) { | ||
| l = nanoTimeToString((Long) l); | ||
| u = nanoTimeToString((Long) u); | ||
| format = DEFAULT_DATE_NANOS_FORMATTER.pattern(); | ||
| } | ||
|
|
||
| if (dataType == IP) { | ||
| if (l instanceof BytesRef bytesRef) { | ||
| l = ipToString(bytesRef); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ | |
| import org.elasticsearch.common.io.stream.Writeable; | ||
| import org.elasticsearch.common.time.DateFormatter; | ||
| import org.elasticsearch.compute.operator.EvalOperator; | ||
| import org.elasticsearch.logging.LogManager; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; | ||
| import org.elasticsearch.xpack.esql.capabilities.TranslationAware; | ||
| import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException; | ||
|
|
@@ -50,14 +52,17 @@ | |
|
|
||
| import static org.elasticsearch.common.logging.LoggerMessageFormat.format; | ||
| import static org.elasticsearch.xpack.esql.core.expression.Foldables.valueOf; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_NANOS; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.IP; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG; | ||
| import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION; | ||
| import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.DEFAULT_DATE_NANOS_FORMATTER; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.DEFAULT_DATE_TIME_FORMATTER; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.HOUR_MINUTE_SECOND; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.commonType; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.dateTimeToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.dateWithTypeToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.ipToString; | ||
| import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.versionToString; | ||
|
|
||
|
|
@@ -66,6 +71,8 @@ public abstract class EsqlBinaryComparison extends BinaryComparison | |
| EvaluatorMapper, | ||
| TranslationAware.SingleValueTranslationAware { | ||
|
|
||
| private static final Logger logger = LogManager.getLogger(EsqlBinaryComparison.class); | ||
|
|
||
| private final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap; | ||
|
|
||
| private final BinaryComparisonOperation functionType; | ||
|
|
@@ -375,14 +382,35 @@ private Query translate(TranslatorHandler handler) { | |
| String format = null; | ||
| boolean isDateLiteralComparison = false; | ||
|
|
||
| logger.trace( | ||
| "Translating binary comparison with right: [" | ||
| + right() | ||
| + "<" | ||
| + right().dataType() | ||
| + ">] left: [" | ||
| + left() | ||
| + "<" | ||
| + left().dataType() | ||
| + ">] attribute: [" | ||
| + attribute | ||
| + "<" | ||
| + attribute.dataType() | ||
| + ">]" | ||
|
||
| ); | ||
|
|
||
| // TODO: This type coersion layer is copied directly from the QL counterpart code. It's probably not necessary or desireable | ||
| // in the ESQL version. We should instead do the type conversions using our casting functions. | ||
| // for a date constant comparison, we need to use a format for the date, to make sure that the format is the same | ||
| // no matter the timezone provided by the user | ||
| if (value instanceof ZonedDateTime || value instanceof OffsetTime) { | ||
| DateFormatter formatter; | ||
| if (value instanceof ZonedDateTime) { | ||
| formatter = DEFAULT_DATE_TIME_FORMATTER; | ||
| // NB: we check the data type of right here because value is the RHS value | ||
| formatter = switch (right().dataType()) { | ||
| case DATETIME -> DEFAULT_DATE_TIME_FORMATTER; | ||
| case DATE_NANOS -> DEFAULT_DATE_NANOS_FORMATTER; | ||
| default -> throw new EsqlIllegalArgumentException("Found date value in non-date type comparison"); | ||
| }; | ||
| // RangeQueryBuilder accepts an Object as its parameter, but it will call .toString() on the ZonedDateTime instance | ||
| // which can have a slightly different format depending on the ZoneId used to create the ZonedDateTime | ||
| // Since RangeQueryBuilder can handle date as String as well, we'll format it as String and provide the format as well. | ||
|
|
@@ -408,10 +436,14 @@ private Query translate(TranslatorHandler handler) { | |
| } | ||
|
|
||
| ZoneId zoneId = null; | ||
| if (DataType.isDateTime(attribute.dataType())) { | ||
| if (attribute.dataType() == DATETIME) { | ||
| zoneId = zoneId(); | ||
| value = dateTimeToString((Long) value); | ||
| value = dateWithTypeToString((Long) value, right().dataType()); | ||
| format = DEFAULT_DATE_TIME_FORMATTER.pattern(); | ||
| } else if (attribute.dataType() == DATE_NANOS) { | ||
| zoneId = zoneId(); | ||
| value = dateWithTypeToString((Long) value, right().dataType()); | ||
| format = DEFAULT_DATE_NANOS_FORMATTER.pattern(); | ||
| } | ||
| if (this instanceof GreaterThan) { | ||
| return new RangeQuery(source(), name, value, false, null, false, format, zoneId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,8 @@ | |
| import org.elasticsearch.index.query.QueryBuilders; | ||
| import org.elasticsearch.index.query.SearchExecutionContext; | ||
| import org.elasticsearch.index.search.NestedHelper; | ||
| import org.elasticsearch.logging.LogManager; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.search.fetch.StoredFieldsSpec; | ||
| import org.elasticsearch.search.internal.AliasFilter; | ||
| import org.elasticsearch.search.lookup.SearchLookup; | ||
|
|
@@ -76,6 +78,8 @@ | |
| import static org.elasticsearch.index.mapper.MappedFieldType.FieldExtractPreference.NONE; | ||
|
|
||
| public class EsPhysicalOperationProviders extends AbstractPhysicalOperationProviders { | ||
| private static final Logger logger = LogManager.getLogger(EsPhysicalOperationProviders.class); | ||
|
|
||
| /** | ||
| * Context of each shard we're operating against. | ||
| */ | ||
|
|
@@ -180,6 +184,7 @@ public Function<org.elasticsearch.compute.lucene.ShardContext, Query> querySuppl | |
| @Override | ||
| public final PhysicalOperation sourcePhysicalOperation(EsQueryExec esQueryExec, LocalExecutionPlannerContext context) { | ||
| final LuceneOperator.Factory luceneFactory; | ||
| logger.trace("Query Exec is " + esQueryExec); | ||
|
||
|
|
||
| List<Sort> sorts = esQueryExec.sorts(); | ||
| assert esQueryExec.estimatedRowSize() != null : "estimated row size not initialized"; | ||
|
|
||
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.
I'd prefer if we added a short description so I don't have to open github when reading the code.