QL: Eliminate internal type DATETIME_NANOS #68220

matriv · 2021-01-29T15:04:56Z

Moving towards grouping of data types in the field caps API
the internal data type DATETIME_NANOS, introduced for date_nanos
support, is eliminated.

Relates: #67722
Follows: #67666

Moving towards grouping of data types in the field caps API the internal data type `DATETIME_NANOS` introduced for `date_nanos` supports is eliminated. Relates: elastic#67722 Follows: elastic#67666

elasticmachine · 2021-01-29T15:04:59Z

Pinging @elastic/es-ql (Team:QL)

costin

Shouldn't the DATE_NANOS type be removed as well, per the PR description?
The main feedback that I have is for this PR to land after adding the transition to fields API.

costin · 2021-02-02T10:10:43Z

.../src/main/java/org/elasticsearch/xpack/eql/execution/search/extractor/FieldHitExtractor.java

-            if (values instanceof String) {
-                return DateUtils.asDateTimeWithNanos(values.toString(), zoneId());
+                try {
+                    return DateUtils.asDateTimeWithNanos(values.toString(), zoneId());


I'd wait for the fields API from @astefan to land before going for this one.
Hopefully that eliminates the internal handling of data format and thus this try/catch as well.

I guess fields API will make this uniform, but just in case, an alternative might be to check the presence of the T or . in the string and based on that call the right parser.

I was curious to see the perf difference of try/catch and trying to check the string:

Benchmark Mode Cnt Score Error Units BenchmarkDateParse.testWithContains thrpt 25 1631774,366 ± 6016,080 ops/s BenchmarkDateParse.testWithException thrpt 25 629767,727 ± 3843,439 ops/s

and it's quite big!

I will use the contains("-") since it's always there for Datetime stings and it's at the beginning of the string.

and it's quite big!

I wonder if indexing the string wouldn't actually make a further difference: contains takes each character at a time, but the datetime format is fixed, so - should always be at .charAt(4). Either way, I think it's fine.

hm, yeah, I rushed here, need to revisit:

a timestamp can start with - (negative millis).

.charAt(4) is also not safe, as there are possible dates like: +14953-02-.... or -134-05-....

matriv · 2021-02-02T10:19:37Z

Shouldn't the DATE_NANOS type be removed as well, per the PR description?
The main feedback that I have is for this PR to land after adding the transition to fields API.

No problem to wait until the fields API transition.

What do you mean DATE_NANOS? There's no such data type introduced in QL/SQL.

mark-vieira · 2021-02-03T00:15:59Z

@elasticmachine update branch

costin · 2021-02-03T08:04:50Z

What do you mean DATE_NANOS? There's no such data type introduced in QL/SQL.

I meant org.elasticsearch.xpack.ql.type.DataTypes.DATETIME_NANOS

matriv · 2021-02-03T12:06:35Z

What do you mean DATE_NANOS? There's no such data type introduced in QL/SQL.

I meant org.elasticsearch.xpack.ql.type.DataTypes.DATETIME_NANOS

https://github.com/elastic/elasticsearch/pull/68220/files#diff-f31cd8869d3c81accd160cec419b0b8ae9308c0dcb4b5c9d38afedc0de6321a6L43

matriv · 2021-02-10T11:17:33Z

@elasticmachine run elasticsearch-ci/2

matriv · 2021-02-10T11:41:25Z

We don't need to extract date_nanos for EQL as we don't have a project phase as in SQL.
Could be needed if we implement support for date_nanos for sequence matching: Opened #68812 to track this.

astefan

LGTM in general. I've left one comment about a method that seems to have sneaked in at merge time.
Also, in ExpressionTests do we still need the testCastToDatetimeNanosNotSupported() test?

astefan · 2021-02-10T13:23:45Z

...ugin/eql/src/main/java/org/elasticsearch/xpack/eql/querydsl/container/SearchHitFieldRef.java

+    private static boolean hasDocValues(DataType dataType) {
+        return dataType == KEYWORD || dataType == DATETIME;
+    }
+


This is not needed.

matriv · 2021-02-10T13:42:24Z

@astefan Nice catch, removing the test, and indeed I missed this merge mistake with the hasDocValues() method, removing.

bpintea

LGTM

astefan

LGTM

astefan · 2021-02-10T15:33:00Z

@elasticmachine run elasticsearch-ci/2

costin

More lines removed than added - that's nice.
Left a comment regarding the history in Github otherwise LGTM.

costin · 2021-02-10T16:08:27Z

.../src/main/java/org/elasticsearch/xpack/eql/execution/search/extractor/FieldHitExtractor.java

-                }
+                // We ask @timestamp (or the defined alternative field) to be returned as `epoch_millis`
+                // when matching sequence to avoid parsing into ZonedDateTime objects for performance reasons.
+                return parseEpochMillisAsString(values.toString());


+1 to this improvement.

costin · 2021-02-10T16:09:49Z

...in/java/org/elasticsearch/xpack/ql/execution/search/extractor/AbstractFieldHitExtractor.java

-
-    private static NumberType numberType(DataType dataType) {
-        return NumberType.valueOf(dataType.esType().toUpperCase(Locale.ROOT));
+        return values;


Isn't the left side removed due to the use of fields API? Just double checking why the change appears to belong to this PR in Github.

It was a merge mistake, it's reverted already.

Moving towards grouping of data types in the field caps API the internal data type `DATETIME_NANOS` introduced for `date_nanos` support is eliminated. Relates: #67722 Follows: #67666 (cherry picked from commit 45677a3)

QL: Eliminate internal type DATETIME_NANOS

b0963f2

Moving towards grouping of data types in the field caps API the internal data type `DATETIME_NANOS` introduced for `date_nanos` supports is eliminated. Relates: elastic#67722 Follows: elastic#67666

matriv added :Analytics/SQL SQL querying v8.0.0 :Analytics/EQL EQL querying v7.12.0 labels Jan 29, 2021

elasticmachine added Team:QL (Deprecated) Meta label for query languages team labels Jan 29, 2021

matriv requested review from costin, astefan and bpintea and removed request for costin January 29, 2021 15:06

matriv added 2 commits January 29, 2021 17:09

add comment

8cf35d5

unused import

45ae90a

elastic deleted a comment from elasticmachine Jan 29, 2021

matriv and others added 3 commits January 29, 2021 17:45

fix docs test

fd46490

Merge remote-tracking branch 'upstream/master' into merge-datetime_nanos

4c596ca

Merge remote-tracking branch 'upstream/master' into merge-datetime_nanos

d774f87

costin reviewed Feb 2, 2021

View reviewed changes

matriv added 2 commits February 5, 2021 12:59

Merge remote-tracking branch 'upstream/master' into merge-datetime_nanos

0d27ae4

eliminate try/catch blocks for sql in extractors for DATETIME

05f2b0e

matriv requested a review from costin February 5, 2021 12:36

matriv added 3 commits February 5, 2021 14:39

unused import

aa2cf91

fix tests

6f432b1

unused imports

827f99c

matriv added 5 commits February 10, 2021 12:31

Merge remote-tracking branch 'upstream/master' into merge-datetime_nanos

17d1d2e

fix merging mistakes

151cf67

date_nanos are never currently extracted

2734aaf

unused import

99b7c25

fix formatting

632c2b9

astefan reviewed Feb 10, 2021

View reviewed changes

Address comments

573cac4

matriv requested a review from astefan February 10, 2021 13:55

matriv and others added 2 commits February 10, 2021 16:03

remove unecessary code related to fullFieldName

0c3747a

Merge remote-tracking branch 'upstream/master' into merge-datetime_nanos

8357e19

bpintea approved these changes Feb 10, 2021

View reviewed changes

astefan approved these changes Feb 10, 2021

View reviewed changes

costin approved these changes Feb 10, 2021

View reviewed changes

matriv merged commit 45677a3 into elastic:master Feb 10, 2021

matriv deleted the merge-datetime_nanos branch February 10, 2021 17:15

williamrandolph added the >non-issue label Feb 18, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QL: Eliminate internal type DATETIME_NANOS #68220

QL: Eliminate internal type DATETIME_NANOS #68220

matriv commented Jan 29, 2021 •

edited

Loading

elasticmachine commented Jan 29, 2021

costin left a comment

costin Feb 2, 2021

bpintea Feb 2, 2021

matriv Feb 5, 2021

matriv Feb 5, 2021

bpintea Feb 5, 2021

matriv Feb 5, 2021

matriv commented Feb 2, 2021 •

edited

Loading

mark-vieira commented Feb 3, 2021

costin commented Feb 3, 2021

matriv commented Feb 3, 2021

matriv commented Feb 10, 2021

matriv commented Feb 10, 2021

astefan left a comment

astefan Feb 10, 2021

matriv commented Feb 10, 2021

bpintea left a comment

astefan left a comment

astefan commented Feb 10, 2021

costin left a comment

costin Feb 10, 2021

costin Feb 10, 2021

matriv Feb 10, 2021

QL: Eliminate internal type DATETIME_NANOS #68220

QL: Eliminate internal type DATETIME_NANOS #68220

Conversation

matriv commented Jan 29, 2021 • edited Loading

elasticmachine commented Jan 29, 2021

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Feb 2, 2021 • edited Loading

mark-vieira commented Feb 3, 2021

costin commented Feb 3, 2021

matriv commented Feb 3, 2021

matriv commented Feb 10, 2021

matriv commented Feb 10, 2021

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Feb 10, 2021

bpintea left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

astefan commented Feb 10, 2021

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Jan 29, 2021 •

edited

Loading

matriv commented Feb 2, 2021 •

edited

Loading