Update TIME and DATE functions by MitchellGale · Pull Request #134 · Bit-Quill/opensearch-project-sql

MitchellGale · 2022-10-13T18:01:20Z

Description

Add option to accept datetime like string in both TIME and DATE (eg. accept "1999-01-02 12:12:12" for both TIME and DATE.

Strict check on date for testing for valid dates (eg. Don't accept Feb 30th as a valid date)

opensearchsql> SELECT TIME("1999-02-30 21:07:32");                                                                                                                                                                                                          
TransportError(500, 'SemanticCheckException', {'error': {'type': 'SemanticCheckException', 'reason': 'Invalid Query', 'details': 'time:1999-02-30 21:07:32 in unsupported format, please use HH:mm:ss[.SSSSSSSSS]'}, 'status': 400})


opensearchsql> SELECT DATE("1999-02-19 21:07:32");                                                                                                                                                                                                          
fetched rows / total rows = 1/1
+-------------------------------+
| DATE("1999-02-19 21:07:32")   |
|-------------------------------|
| 1999-02-19                    |
+-------------------------------+
opensearchsql> SELECT DATE("1999-02-19");                                                                                                                                                                                                                   
fetched rows / total rows = 1/1
+----------------------+
| DATE("1999-02-19")   |
|----------------------|
| 1999-02-19           |
+----------------------+
opensearchsql> SELECT TIME("1999-02-19 21:07:32");                                                                                                                                                                                                          
fetched rows / total rows = 1/1
+-------------------------------+
| TIME("1999-02-19 21:07:32")   |
|-------------------------------|
| 21:07:32                      |
+-------------------------------+
opensearchsql> SELECT TIME("21:07:32");                                                                                                                                                                                                                     
fetched rows / total rows = 1/1
+--------------------+
| TIME("21:07:32")   |
|--------------------|
| 21:07:32           |
+--------------------+

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…y to parse time out of datetime or date out of datetime. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

codecov · 2022-10-13T18:13:20Z

Codecov Report

❗ No coverage uploaded for pull request base (Integ-updateDateFunction@b30d156). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##             Integ-updateDateFunction     #134   +/-   ##
===========================================================
  Coverage                            ?   95.10%           
  Complexity                          ?     3073           
===========================================================
  Files                               ?      303           
  Lines                               ?     8254           
  Branches                            ?      610           
===========================================================
  Hits                                ?     7850           
  Misses                              ?      350           
  Partials                            ?       54

Flag	Coverage Δ
query-workbench	`62.76% <0.00%> (?)`
sql-engine	`97.90% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Yury-Fridlyand

mysql> select DATE(TIME('13:49:00')), TIME(DATE('2020-08-26'));

+------------------------+--------------------------+
| DATE(TIME('13:49:00')) | TIME(DATE('2020-08-26')) |
+------------------------+--------------------------+
| 2022-10-13             | 00:00:00                 |
+------------------------+--------------------------+
1 row in set (0.00 sec)

opensearchsql> select DATE(TIME('13:49:00')), TIME(DATE('2020-08-26'));
{'reason': 'Invalid SQL query', 'details': 'date function expected {[STRING],[DATE],[DATETIME],[TIMESTAMP]}, but get [TIME]', 'type': 'ExpressionEvaluationException'}

Yury-Fridlyand · 2022-10-13T19:36:30Z

I think you need to apply/cherry-pick 40ffca2 into your branch to fix that.

…orresponding tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

…ction.java Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

acarbonetto · 2022-10-14T18:20:04Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

  private ExprValue exprDate(ExprValue exprValue) {
    if (exprValue instanceof ExprStringValue) {
-      return new ExprDateValue(exprValue.stringValue());
+      try {


We should be able to remove the if statement and get the same result. The try...catch will cover both cases.

I think we still need it because it can be a stringValue or a dateValue.

is there a case where an ExprStringValue doesn't have a stringValue()?

acarbonetto · 2022-10-14T18:20:30Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

   */
  private ExprValue exprTime(ExprValue exprValue) {
    if (exprValue instanceof ExprStringValue) {
+      //try catch


acarbonetto · 2022-10-14T18:21:26Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

    assertEquals(new ExprDateValue("2020-08-17"), eval(expr));
    assertEquals("date(DATE '2020-08-17')", expr.toString());
+
+    expr = dsl.date(DSL.literal(new ExprDateValue("2020-08-17 12:12:00")));


we should test with nano seconds too

acarbonetto · 2022-10-14T18:21:45Z

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

    assertEquals(new ExprTimeValue("01:01:01"), eval(expr));
    assertEquals("time(TIME '01:01:01')", expr.toString());
+
+    expr = dsl.time(DSL.literal(new ExprTimeValue("2020-01-02 01:01:01")));


we should test with nano seconds too (separately)

Added case for times < 1.0 seconds. 2016222

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand · 2022-10-19T18:58:09Z

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java

+
+  public static final DateTimeFormatter TIME_FORMATTER_VARIABLE_NANOS_OPTIONAL =
+      new DateTimeFormatterBuilder()
+          .appendPattern("[uuuu-MM-dd HH:mm:ss][HH:mm:ss]")


Do you have tests for both patterns?

All patterns should have coverage

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

…E functions. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

acarbonetto · 2022-10-20T20:26:34Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

   * @return ExprValue.
   */
  private ExprValue exprDate(ExprValue exprValue) {
+    if (exprValue.type() == TIME) {


why not use a switch on type() here. For TIME, STRING, DATE, and TIMESTAMP?

acarbonetto · 2022-10-20T20:27:00Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

    if (exprValue instanceof ExprStringValue) {
-      return new ExprTimeValue(exprValue.stringValue());
+      try {
+        return new ExprTimeValue(exprValue.stringValue());


why not use a switch on type() here. For TIME, STRING, DATE, and TIMESTAMP?

Yury-Fridlyand · 2022-10-25T15:50:51Z

@MitchellGale-BitQuill,
can you add support for TIME(HH:mm)? Current format requires seconds as a mandatory part in input string.

…cal time now. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

MitchellGale · 2022-10-27T18:15:15Z

@Yury-Fridlyand Added support for HH:mm in 61289bb.

MitchellGale · 2022-10-27T19:03:04Z

@[MitchellGale-BitQuill](https://github.com/MitchellGale-BitQuill) MitchellGale-BitQuill changed the title [BLOCKED] Update TIME and DATE functions Update TIME and DATE functions [now](https://github.com/Bit-Quill/opensearch-project-sql/pull/134#event-7685087474)

Removed blocking component. It can be fixed in a later PR.

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand · 2022-10-28T18:25:25Z

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

  @Override
  public Instant timestampValue() {
-    return ZonedDateTime.of(date, timeValue(), ZoneId.systemDefault()).toInstant();
+    return ZonedDateTime.of(date, timeValue(), ZoneId.of("UTC")).toInstant();


Last minor change - you need to revert this as well ... or ... update test to use UTC too.

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

…prDateValue.java for timestampValue to use systemDefault instead of UTC time. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Added test cases and support for strict date validation. Added abilit…

65c34ac

…y to parse time out of datetime or date out of datetime. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

MitchellGale changed the title ~~Added test cases and support for strict date validation. Added abilit…~~ Update TIME and DATE functions Oct 13, 2022

Yury-Fridlyand requested review from GabeFernandez310, GumpacG, MaxKsyunz, Yury-Fridlyand, acarbonetto and forestmvey October 13, 2022 18:16

GumpacG approved these changes Oct 13, 2022

View reviewed changes

forestmvey approved these changes Oct 13, 2022

View reviewed changes

Yury-Fridlyand suggested changes Oct 13, 2022

View reviewed changes

Update ExprTimeValue by datetimeValue and other interfaces. Add c…

4a4e425

…orresponding tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand mentioned this pull request Oct 14, 2022

SQL ingestion and ops - standup opensearch-project/sql#791

Closed

Yury-Fridlyand added 2 commits October 14, 2022 10:55

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFun…

da22234

…ction.java Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fix conversion to timestamp.

e421349

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

acarbonetto approved these changes Oct 14, 2022

View reviewed changes

MitchellGale added 5 commits October 17, 2022 09:52

Added try catch to return null for invalid date in DATE

26c1165

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added null test for TIME and DATE

be3ea22

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added exprDate to accept time.

4f7edd1

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Accept date time for time and date

a00863c

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Adding coverage

99c98b0

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand reviewed Oct 19, 2022

View reviewed changes

Yury-Fridlyand suggested changes Oct 19, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/utils/DateTimeFormatters.java Outdated Show resolved Hide resolved

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java Outdated Show resolved Hide resolved

MitchellGale added 4 commits October 19, 2022 13:28

Fixed code coverage for time passed to date function case.

f9e38e9

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Adding more test cases for datetime and milliseconds for DATE and TIM…

2016222

…E functions. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Removed unused imports.

4c0928d

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Removed unused DATETIMEFORMATTERS.

95c5e5c

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

acarbonetto reviewed Oct 20, 2022

View reviewed changes

Yury-Fridlyand self-requested a review October 20, 2022 22:13

acarbonetto changed the title ~~Update TIME and DATE functions~~ [BLOCKED] Update TIME and DATE functions0 Oct 25, 2022

acarbonetto changed the title ~~[BLOCKED] Update TIME and DATE functions0~~ [BLOCKED] Update TIME and DATE functions Oct 25, 2022

Added support for HH:mm for time/datetime inputs. Removed call for lo…

61289bb

…cal time now. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand approved these changes Oct 26, 2022

View reviewed changes

Yury-Fridlyand self-requested a review October 26, 2022 00:47

Added more doc tests.

310f653

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

MitchellGale changed the title ~~[BLOCKED] Update TIME and DATE functions~~ Update TIME and DATE functions Oct 27, 2022

Yury-Fridlyand reviewed Oct 28, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java Outdated Show resolved Hide resolved

Yury-Fridlyand self-requested a review October 28, 2022 01:09

Removed blocks from cherry-picked PR.

484b28c

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand suggested changes Oct 28, 2022

View reviewed changes

MitchellGale added 2 commits October 28, 2022 13:23

Reverted change in ExprDateTimeValue

157059f

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Reverted reverted changes in ExprDateTimeValue. Reverted change in Ex…

77bd46e

…prDateValue.java for timestampValue to use systemDefault instead of UTC time. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Yury-Fridlyand approved these changes Oct 28, 2022

View reviewed changes

MitchellGale merged commit bc1507c into Integ-updateDateFunction Oct 31, 2022

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Update to Gradle 7.5.1 (#134)

9b4e9a6

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Conversation

MitchellGale commented Oct 13, 2022

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand commented Oct 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand commented Oct 25, 2022

Uh oh!

MitchellGale commented Oct 27, 2022

Uh oh!

MitchellGale commented Oct 27, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Oct 13, 2022 •

edited

Loading