Minor clean up of datetime and other classes #198

MitchellGale · 2022-12-22T01:42:31Z

Description

Clean up lexer to make future changes easier. Make changes to documentation and code to make cleaner.

Groom ANTLR grammar files to make future changes easier.
Fix EqualsAndHashCode compilation warnings

Generating equals/hashCode implementation but without a call to superclass, even though this class does not extend java.lang.Object. If this is intentional, add '@EqualsAndHashCode(callSuper=false)' to your type.

Move ExprTimestampValue.ZONE constant to DateTimeUtils
Move ExprDatetimeValue formatter to DateTimeFormatters
Sort alphabetically functions in DateTimeFunction and MathematicalFunction
Replace FunctionDSL import by static imports to simplify code
Remove tests for NULL and MISSING values
Add validation for non-zero divider
Remove Environment mock from some unit tests, because it is not needed anymore
Groom docs to eliminate rst validator warnings
Add doctests for some functions
Make IT for now-like functions truly parametrized
Rename some PPL grammar entities to be consistent with SQL ones

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.

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

GabeFernandez310 · 2023-01-17T23:38:09Z

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

@@ -151,4 +151,6 @@ public static LocalDate extractDate(ExprValue value,
        ? ((ExprTimeValue) value).dateValue(functionProperties)
        : value.dateValue();
  }
+
+  public static final ZoneId UTC_ZONE_ID = ZoneId.of("UTC");


Should constants be at the beginning or at the end?

Do we also want to "encourage" private functions to be defined after public functions?

GabeFernandez310 · 2023-01-17T23:40:58Z

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

    assertEquals("adddate(\"2020-08-26\", interval(1, \"day\"))", expr.toString());
-
-    when(nullRef.type()).thenReturn(DATE);


I'm curious on what these removed asserts actually do. I noticed them as I've been working on DateTime stuff, but I don't actually know what they are supposed to be testing.

They handle nullMissingHandling, which is tested in FunctionDSLnullMissingHandlingTest. Thanks Max by adding those tests.

This specific line mocks type of nullRef to let repository pick a function signature.
The null/missing validation actually doesn't test the function and it is not needed to repeat it 100500 times for each function.

GabeFernandez310 · 2023-01-17T23:44:14Z

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

@@ -444,10 +319,6 @@ private void testDayOfMonthWithUnderscores(FunctionExpression dateExpression, in

  @Test
  public void dayOfMonthWithUnderscores() {
-    lenient().when(nullRef.valueOf(env)).thenReturn(nullValue());


These will probably make my PR 204 for time_format conflict. Is it safe to checkout this file into my PR, or should I wait until this is merged upstream and rebase before making my PR for time_format upstream.

It doesn't matter which PR goes first, but we have to rebase second one once first merged and fix conflicts.

GabeFernandez310 · 2023-01-17T23:55:45Z

docs/user/ppl/functions/datetime.rst

-    | '13:14:15.012345'                             | '1998-Jan-31st 01:14:15 PM'                                    |
-    +-----------------------------------------------+----------------------------------------------------------------+
+    +------------------------------------------------------+-----------------------------------------------------------------------+
+    | DATE_FORMAT('1998-01-31 13:14:15.012345', '%T.%f')   | DATE_FORMAT(TIMESTAMP('1998-01-31 13:14:15.012345'), '%Y-%b-%D %r')   |


I have also changed this in my PR. Should I revert my changes?

No!
That would be interpreted as merge conflict, which is easy to resolve.

...ava/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQuery.java

…d on a doctest cluster. Signed-off-by: Yury-Fridlyand <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

core/src/main/java/org/opensearch/sql/ast/statement/Explain.java

acarbonetto · 2023-01-19T06:24:13Z

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

@@ -40,7 +36,7 @@ public class ExprTimestampValue extends AbstractExprValue {
  public ExprTimestampValue(String timestamp) {
    try {
      this.timestamp = LocalDateTime.parse(timestamp, DATE_TIME_FORMATTER_VARIABLE_NANOS)
-          .atZone(ZONE)
+          .atZone(UTC_ZONE_ID)


Is there any interest in including a default time zone in the functionalProperties object?

acarbonetto · 2023-01-19T06:26:27Z

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

@@ -151,4 +151,6 @@ public static LocalDate extractDate(ExprValue value,
        ? ((ExprTimeValue) value).dateValue(functionProperties)
        : value.dateValue();
  }
+
+  public static final ZoneId UTC_ZONE_ID = ZoneId.of("UTC");


Do we also want to "encourage" private functions to be defined after public functions?

Signed-off-by: Yury-Fridlyand <[email protected]>

Signed-off-by: MitchellGale-BitQuill <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

…ct#1310) * Minor clean up of datetime and other classes (#198) --------- Signed-off-by: MitchellGale-BitQuill <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

…ct#1310) (opensearch-project#1567) * Minor clean up of datetime and other classes (#198) --------- Signed-off-by: MitchellGale-BitQuill <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> (cherry picked from commit bc29a8a) Co-authored-by: Yury-Fridlyand <[email protected]>

This comment was marked as spam.

Sign in to view

MitchellGale changed the base branch from Integ-cleanUp to Integ-clean_up December 22, 2022 05:30

MitchellGale changed the base branch from Integ-clean_up to Integ-cleanUp December 22, 2022 05:30

MitchellGale changed the base branch from Integ-cleanUp to Integ-cleanup2 December 22, 2022 05:31

MitchellGale changed the base branch from Integ-cleanup2 to integ-cleanupcode December 22, 2022 05:39

MitchellGale and others added 9 commits January 16, 2023 10:57

Cleaned up PPL and SQL Parser.

5590581

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Added changes

03b663f

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Made constant for UTC_ZONE_ID

0d5095f

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Made constant for UTC_ZONE_ID and other clean up

db81a4b

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Checkstyle fixes

1a9d482

Signed-off-by: MitchellGale-BitQuill <[email protected]>

Fix rebase error.

292370c

Signed-off-by: Yury-Fridlyand <[email protected]>

Revert some changes - PPL math operation precedence was broken.

b11b239

Signed-off-by: Yury-Fridlyand <[email protected]>

Groom datetime functions unit tests.

2ca51bb

Signed-off-by: Yury-Fridlyand <[email protected]>

Update and groom docs.

f3ea329

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand force-pushed the dev-DateTime_cleaning branch from 56a89a7 to f3ea329 Compare January 17, 2023 01:21

Yury-Fridlyand added 3 commits January 17, 2023 09:32

Fix compilation warnings for EqualsAndHashCode.

18e7e21

Signed-off-by: Yury-Fridlyand <[email protected]>

Fix checkstyle

553b61d

Signed-off-by: Yury-Fridlyand <[email protected]>

Minor doc fix.

1c46fe8

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand marked this pull request as ready for review January 17, 2023 19:50

GabeFernandez310 reviewed Jan 17, 2023

View reviewed changes

GabeFernandez310 reviewed Jan 18, 2023

View reviewed changes

...ava/org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQuery.java Show resolved Hide resolved

GabeFernandez310 approved these changes Jan 18, 2023

View reviewed changes

Yury-Fridlyand added 5 commits January 17, 2023 21:10

Deactivate doctests for legacy engine, because they cannot be execute…

11da1f3

…d on a doctest cluster. Signed-off-by: Yury-Fridlyand <[email protected]>

Simplify FunctionDSL imports and remove null/missing arg tests.

6bd65f0

Signed-off-by: Yury-Fridlyand <[email protected]>

Reactivate tests disabled in 11da1f3. Change doctest cluster type.

5eea3d2

Signed-off-by: Yury-Fridlyand <[email protected]>

Code style fixes.

939f916

Signed-off-by: Yury-Fridlyand <[email protected]>

Minor ANTLR grammar grooming.

159ce3d

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested review from acarbonetto, forestmvey, GumpacG, margarit-h, matthewryanwells and MaxKsyunz January 18, 2023 23:45

Make IT for now-like functions truly parametrized.

6bbabb9

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto reviewed Jan 19, 2023

View reviewed changes

core/src/main/java/org/opensearch/sql/ast/statement/Explain.java Show resolved Hide resolved

acarbonetto reviewed Jan 19, 2023

View reviewed changes

acarbonetto approved these changes Jan 19, 2023

View reviewed changes

GumpacG approved these changes Jan 19, 2023

View reviewed changes

Rename some PPL grammar entities to be consistent with SQL ones.

12aab06

Signed-off-by: Yury-Fridlyand <[email protected]>

matthewryanwells approved these changes Jan 30, 2023

View reviewed changes

Yury-Fridlyand merged commit a6b4f48 into integ-cleanupcode Jan 30, 2023

Yury-Fridlyand deleted the dev-DateTime_cleaning branch January 30, 2023 21:05

Yury-Fridlyand mentioned this pull request Jan 30, 2023

Minor clean up of datetime and other classes (#198) opensearch-project/sql#1310

Merged

6 tasks

Yury-Fridlyand added a commit that referenced this pull request Feb 7, 2023

Minor clean up of datetime and other classes (#198)

e295b2b

Signed-off-by: MitchellGale-BitQuill <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand mentioned this pull request Jul 20, 2023

Fix ASCII function and groom UT for text functions. #301

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor clean up of datetime and other classes #198

Minor clean up of datetime and other classes #198

MitchellGale commented Dec 22, 2022 •

edited by Yury-Fridlyand

Loading

This comment was marked as spam.

GabeFernandez310 Jan 17, 2023

acarbonetto Jan 19, 2023

GabeFernandez310 Jan 17, 2023

Yury-Fridlyand Jan 18, 2023

GabeFernandez310 Jan 17, 2023

Yury-Fridlyand Jan 18, 2023

GabeFernandez310 Jan 17, 2023

Yury-Fridlyand Jan 18, 2023

acarbonetto Jan 19, 2023

acarbonetto Jan 19, 2023

		assertEquals("adddate(\"2020-08-26\", interval(1, \"day\"))", expr.toString());

		when(nullRef.type()).thenReturn(DATE);

Minor clean up of datetime and other classes #198

Minor clean up of datetime and other classes #198

Conversation

MitchellGale commented Dec 22, 2022 • edited by Yury-Fridlyand Loading

Description

Check List

This comment was marked as spam.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellGale commented Dec 22, 2022 •

edited by Yury-Fridlyand

Loading