-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
SQL: Extend DATE_TRUNC to also operate on intervals(elastic - #46632 ) #47720
Conversation
Pinging @elastic/es-search (:Search/SQL) |
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.
First of all: thank you for your contribution!
Nice job so far, I left some comments.
Moreover, I'd like to ask you to update the documentation of the function here:
https://github.com/elastic/elasticsearch/master/docs/reference/sql/functions/date-time.asciidoc regarding the 2nd argument type (add the interval support) along with some example/integ tests
here: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec#L2546
Please also add some integration tests here: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec#L143 and if possible also some for filter/groupby/having/orderby below in the same file.
Thank you!
...java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessor.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessorTests.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessorTests.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
@matriv thank you for review. I will fix the problems soon as possible. |
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.
Nice work, but it needs a serious formatting step before merging. I've added some comments, as well.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/TypeResolutions.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessor.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessorTests.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
Hi @astefan Thank you very much for your kind review. I started to work on problems. |
@matriv |
@musteaf currently your PR is changed to point to your master branch:
and we can see no changes anymore. Our parent class which loads the test_emp data is |
Hi @matriv, This is the query:
and error
Do you have any suggestion? |
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.
@musteaf Thank you for addressing our comments, I left some new though, most of them minor.
Regarding the integ test for GROUP BY, how about not using an alias but repeat the function in the GROUP BY:
SELECT count(*) as count, birth_date + DATE_TRUNC('decade', INTERVAL '1 12:43:21' DAY TO SECONDS) AS dt FROM test_emp GROUP BY birth_date + DATE_TRUNC('decade', INTERVAL '1 12:43:21' DAY TO SECONDS) ORDER BY 2 LIMIT 3;
or even without the alias:
SELECT count(*) as count, birth_date + DATE_TRUNC('decade', INTERVAL '1 12:43:21' DAY TO SECONDS) FROM test_emp GROUP BY birth_date + DATE_TRUNC('decade', INTERVAL '1 12:43:21' DAY TO SECONDS) ORDER BY 2 LIMIT 3;
If neither work please add the test as is but with -Ignore
as a suffix and right above it add a comment:
// AwaitsFix https://github.com/elastic/elasticsearch/issues/42041
so we can enable it once the bug is fixed.
.../src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DatePart.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTrunc.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessor.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessorTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine test this please |
@matriv Thank you for your kind review. I really learned a lot from this PR. Thank you for help. I fixed the issues. But, unfortunately GROUP BY doesn't work. |
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 left some more comments, please check.
...java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTruncProcessor.java
Outdated
Show resolved
Hide resolved
...org/elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
Outdated
Show resolved
Hide resolved
@elasticmachine test this please |
Don't worry, thx for adding the test as ignored, it will be enabled once the bug is fixed. |
@elasticmachine run elasticsearch-ci/2 |
@musteaf Please check and fix the checkstyle errors:
|
@matriv Thank you for review. I think I fixed the issues. I left comment about |
@elasticmachine test this please |
@matriv I merged the master and enabled the group by integration test. |
@musteaf Please apply the change we discussed in the description here: #47720 (comment) Please also add references to the 3 examples in the docs.csv-spec file you added to the ascii doc after the examples with Date. |
@elasticmachine test this please. |
|
@elasticmachine test this please |
@musteaf Please run |
@matriv I fixed the issues. Also if there is a "gradlew" command to check styles, could you share with me for further PR? According to the CONTRIBUTING file, we can use Spotless Gradle plugin to check style but It only supports Eclipse. If there is not any "gradlew" command which works with Intellij, I will use Eclipse then. Thanks in advance. |
@musteaf I've pasted it earlier: |
@elasticmachine test this please |
@matriv thank you so much for the code snippet. I examined elasticmachine's build errors but I could not find any related error with this PR. |
@elasticmachine update branch |
@elasticmachine test this please |
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.
LGTM
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.
LGTM. Left just few cosmetics related comments.
int year = period.getYears(); | ||
int firstYearOfCentury = year - (year % 100); | ||
return new IntervalYearMonth(Period.ZERO.plusYears(firstYearOfCentury), iym.dataType()); |
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.
Wrong indentation.
import static org.elasticsearch.xpack.ql.util.DateUtils.SECONDS_PER_MINUTE; | ||
import static org.elasticsearch.xpack.sql.expression.SqlTypeResolutions.isDateOrInterval; | ||
import static org.elasticsearch.xpack.sql.type.SqlDataTypes.isInterval; | ||
|
||
public class DateTrunc extends BinaryDateTimeFunction { | ||
|
||
public enum Part implements DateTimeField { | ||
|
||
MILLENNIUM(dt -> { |
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.
Maybe a personal preference, but I think the Part enum values are more easily distinguishable if they are indented in a similar style to this: https://gist.github.com/astefan/a4c27b9bfefb1b074156c86d01d507ff.
At the moment, this is a big block of code where, every few lines, there is a capital letters noun without any clear boundaries.
} | ||
if (truncateDateField == Part.WEEK && (timestamp instanceof IntervalDayTime || timestamp instanceof IntervalYearMonth)) { | ||
throw new SqlIllegalArgumentException("Truncating intervals is not supported for {} units", | ||
truncateTo); |
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.
This truncateTo
can sit on the previous line.
@musteaf Could you please address those last comments? |
@elasticmachine test this please |
@musteaf Thank you for contribution! |
@matriv Thank you so much for your excellent guidance! That was so educative for me. |
#47720) (#53972) The function is extended to operate on intervals according to the PostgreSQL: https://www.postgresql.org/docs/9.1/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC Closes : #46632 (cherry picked from commit 2dc7950) Co-authored-by: musteaf <gs_mustea@hotmail.com>
The function is extended to operate on intervals according to the PostgreSQL: https://www.postgresql.org/docs/9.1/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC
Closes : #46632