Use query execution start time as the value of now-like functions. #149

MaxKsyunz · 2022-10-31T23:24:55Z

Description

Implement now-family of functions using FunctionProperties object.
This object is created at query execution start.
This ensures that each call to now in SELECT now(), now(), now() will return the same value..

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.

Add QueryContext interface to capture query metadata and provide to function implementations. Update FunctionBuilder to take QueryContext in addition to function arguments when evaluating a SQL function. Implement constant date time functions using QueryContext. Signed-off-by: MaxKsyunz <[email protected]>

- Address checkstyle errors - Add comments to QueryContext. - Address PR feedback. Signed-off-by: MaxKsyunz <[email protected]>

1. Rename QueryContext to FunctionProperties, renmae related parameter names. 2. Remove QueryContext interface. 3. Add FunctionProperties as a bean to ExpressionConfig. 4. Add unit tests for ExpressionConfig. Refactored unit tests in opensearch module to keep ExpressionCOnfig misuse in one place -- OpenSearchTestBase. Signed-off-by: MaxKsyunz <[email protected]>

Signed-off-by: MaxKsyunz <[email protected]>

# Conflicts: # core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java # core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeTestBase.java

Make FunctionProperties serializable -- required toString override. Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand · 2022-10-31T23:46:35Z

Are you planning to partially revert changes done in #113? You can't prove that now() returns the same value, because it is cached in ExpressionAnalyzer.

Yury-Fridlyand

Need to update UNIX_TIMESTAMP function and its tests - opensearch-project#996.

codecov · 2022-11-01T01:32:02Z

Codecov Report

Merging #149 (5bed983) into integ-constant-now (be4512e) will decrease coverage by 2.58%.
The diff coverage is 100.00%.

@@                   Coverage Diff                    @@
##             integ-constant-now     #149      +/-   ##
========================================================
- Coverage                 98.26%   95.68%   -2.59%     
- Complexity                 3325     3329       +4     
========================================================
  Files                       324      335      +11     
  Lines                      8399     9053     +654     
  Branches                    553      671     +118     
========================================================
+ Hits                       8253     8662     +409     
- Misses                      142      334     +192     
- Partials                      4       57      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.26% <100.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...a/org/opensearch/sql/analysis/AnalysisContext.java	`100.00% <ø> (ø)`
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <ø> (ø)`
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <ø> (ø)`
...sql/expression/aggregation/AggregatorFunction.java	`100.00% <100.00%> (ø)`
...search/sql/expression/config/ExpressionConfig.java	`100.00% <100.00%> (ø)`
...arch/sql/expression/datetime/DateTimeFunction.java	`100.00% <100.00%> (ø)`
...expression/function/BuiltinFunctionRepository.java	`100.00% <100.00%> (ø)`
...pensearch/sql/expression/function/FunctionDSL.java	`100.00% <100.00%> (ø)`
...ch/sql/expression/function/FunctionProperties.java	`100.00% <100.00%> (ø)`
...expression/function/RelevanceFunctionResolver.java	`100.00% <100.00%> (ø)`
... and 15 more

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

Add unit tests for FunctionProperties, including ensuring it is serializable. Fix serializability of return value of FunctionDSL.implWithProperties Signed-off-by: MaxKsyunz <[email protected]>

Signed-off-by: MaxKsyunz <[email protected]>

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

core/src/main/java/org/opensearch/sql/expression/function/FunctionProperties.java

Signed-off-by: MaxKsyunz <[email protected]>

Constant value caching is no longer necessary in ExpressionAnalyzer -- the same behavior is now implemented with FunctionProperties. Signed-off-by: MaxKsyunz <[email protected]>

On Windows, getQueryStartClock_differs_from_instantNow fails because Instant.now() in the test returns the same value as Instant.now() called for FunctionProperties construction. Signed-off-by: MaxKsyunz <[email protected]>

sql/src/main/antlr/OpenSearchSQLParser.g4

It's failing on Windows -- turns out the assumption about Spring it was testing does not hold true across platforms. Signed-off-by: MaxKsyunz <[email protected]>

core/src/test/java/org/opensearch/sql/expression/function/FunctionDSLTestBase.java

Yury-Fridlyand

I'm glad to see major test grooming here. Can you add brief what and why was changed? Thanks!

Yury-Fridlyand · 2022-11-02T21:32:03Z

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java

            }
          };
      return Pair.of(functionSignature, functionBuilder);
    };
  }

  /**
-   * Unary Function Implementation.
+   * Implementation of no args function that uses FunctionProperties.


Can you swap no-args-function and one-arg-function back? Just to smaller diff.

Yury-Fridlyand · 2022-11-02T21:42:46Z

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

+
+  private LocalDateTime getExpectedNow() {
+    return LocalDateTime.now(
+            functionProperties.getQueryStartClock().withZone(ZoneId.of("UTC")))


Should we use ExprTimestampValue.ZONE here instead?

core/src/test/java/org/opensearch/sql/expression/system/SystemFunctionsTest.java

Yury-Fridlyand · 2022-11-02T21:45:44Z

opensearch/src/test/java/org/opensearch/sql/opensearch/OpenSearchTestBase.java

+  {
+    ExpressionConfig config = new ExpressionConfig();
+    dsl = config.dsl(config.functionRepository(config.functionExecutionContext()));
+  }


Can you make dsl Autowired?

If you name variable DSL (uppercase) you can avoid hundreds changes in inherited test classes.

If you name variable DSL (uppercase) you can avoid hundreds changes in inherited test classes.

namedArgument is a static method -- it works fine without an instance of DSL class. With this updated it's clearer where DSL instance is required and therefore which methods need Spring.

Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand · 2022-11-04T19:19:32Z

Is it supposed to fix conversion ExprTimeValue -> LocalDate, LocalDateTime and Instant in scope of this PR?

MaxKsyunz · 2022-11-05T06:38:43Z

Is it supposed to fix conversion ExprTimeValue -> LocalDate, LocalDateTime and Instant in scope of this PR?

Don't think so?I only made changes sufficient to fix failing tests.

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

Yury-Fridlyand · 2022-11-07T17:50:25Z

Is it possible to use implWithProperties with nullMissingHandling? Something like implWithProperties(nullMissingHandling(λ), ...)

Yury-Fridlyand · 2022-11-07T20:02:58Z

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

+   * `fsp` argument support is removed until refactoring to avoid bug where `now()`, `now(x)` and
+   * `now(y) return different values.
+   */
+  private FunctionResolver now(FunctionName functionName) {


Any reason to return FunctionResolver, but not DefaultFunctionResolver? Does this matter?

FunctionResolver is an interface and DefaultFunctionResolver is an implementation.
Standard practice is to return the most generic type possible.

Should we groom then to return FunctionResolver everywhere in DateTimeFunction?

Yury-Fridlyand · 2022-11-07T20:10:20Z

sql/src/test/java/org/opensearch/sql/sql/parser/AstNowLikeFunctionTest.java

+        "localtime",
+         "now",


Didn't checkstyle blame indents there?

MaxKsyunz · 2022-11-07T23:48:53Z

Is it possible to use implWithProperties with nullMissingHandling? Something like implWithProperties(nullMissingHandling(λ), ...)

Good point! I'll add that.

Add FunctionDSL.nullMissingHandlingWithProperties to all for consistent null and missing value handling across all functions. Signed-off-by: MaxKsyunz <[email protected]>

- Add FunctionProperties interface to capture query metadata and provide to function implementations. - Update FunctionBuilder to take FunctionProperties in addition to function arguments when evaluating a SQL function. - Implement now-like functions using FunctionProperties. - Add FunctionDSL.nullMissingHandlingWithProperties to allow for consistent null and missing value handling across all functions. - Remove constant value caching from ExpressionAnalyzer -- the same behavior is now implemented with FunctionProperties. ### Unit Tests - Adjust getQueryStartClock_differs_from_instantNow unit test. On Windows, getQueryStartClock_differs_from_instantNow fails because Instant.now() in the test returns the same value as Instant.now() called for FunctionProperties construction. - Add unit tests for FunctionDSL. - Use Spring to instantiate dsl in OpenSearchTestBase. Signed-off-by: MaxKsyunz <[email protected]>

MitchellGale · 2022-11-10T23:17:23Z

Validated ✅ LGTM

…notes for 2.3.0 version. (#149) Signed-off-by: Navneet Verma <[email protected]> Signed-off-by: Navneet Verma <[email protected]>

MaxKsyunz added 8 commits October 19, 2022 22:33

Code clean-up

8c8dca9

- Address checkstyle errors - Add comments to QueryContext. - Address PR feedback. Signed-off-by: MaxKsyunz <[email protected]>

Add unit tests for FunctionDSL.

73e8ab1

Signed-off-by: MaxKsyunz <[email protected]>

Complete coverage of FunctionDSL with unit tests

48d077d

Signed-off-by: MaxKsyunz <[email protected]>

Refactor FunctionDSL to remove duplicate code.

be77a7b

Signed-off-by: MaxKsyunz <[email protected]>

Merge remote-tracking branch 'upstream/2.x' into dev-constant-now

7d602c8

# Conflicts: # core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java # core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeTestBase.java

Sync with upstream/2.x

47455e7

Make FunctionProperties serializable -- required toString override. Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz requested review from acarbonetto, GabeFernandez310, GumpacG, margarit-h, MitchellGale, Yury-Fridlyand and a team October 31, 2022 23:25

Yury-Fridlyand requested changes Oct 31, 2022

View reviewed changes

MaxKsyunz added 3 commits November 1, 2022 13:07

Unit tests and serialization

ef4c6e3

Add unit tests for FunctionProperties, including ensuring it is serializable. Fix serializability of return value of FunctionDSL.implWithProperties Signed-off-by: MaxKsyunz <[email protected]>

UNIXTIMESTAMP to use FunctionProperties

b061309

Signed-off-by: MaxKsyunz <[email protected]>

Minor cleanup for FunctionPropertiesTest and FunctionDSL.

7dcbed1

Signed-off-by: MaxKsyunz <[email protected]>

acarbonetto approved these changes Nov 1, 2022

View reviewed changes

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

core/src/main/java/org/opensearch/sql/expression/function/FunctionProperties.java Show resolved Hide resolved

MaxKsyunz added 3 commits November 1, 2022 14:56

Fix UNIXTIMESTAMP related unit tests.

474a425

Signed-off-by: MaxKsyunz <[email protected]>

Remove constant value caching from ExpressionAnalyzer

93966fb

Constant value caching is no longer necessary in ExpressionAnalyzer -- the same behavior is now implemented with FunctionProperties. Signed-off-by: MaxKsyunz <[email protected]>

Adjust getQueryStartClock_differs_from_instantNow unit test

c532644

On Windows, getQueryStartClock_differs_from_instantNow fails because Instant.now() in the test returns the same value as Instant.now() called for FunctionProperties construction. Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand reviewed Nov 2, 2022

View reviewed changes

sql/src/main/antlr/OpenSearchSQLParser.g4 Outdated Show resolved Hide resolved

Remove ExpressionConfigTest.testNotTheSame

c0c3188

It's failing on Windows -- turns out the assumption about Spring it was testing does not hold true across platforms. Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz commented Nov 2, 2022

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/function/FunctionDSLTestBase.java Show resolved Hide resolved

MaxKsyunz marked this pull request as ready for review November 2, 2022 20:13

Yury-Fridlyand self-requested a review November 2, 2022 21:26

Yury-Fridlyand approved these changes Nov 2, 2022

View reviewed changes

Yury-Fridlyand self-requested a review November 2, 2022 21:27

Yury-Fridlyand reviewed Nov 2, 2022

View reviewed changes

MaxKsyunz added 2 commits November 3, 2022 23:59

Merge with upstream/2.x

64d255c

Signed-off-by: MaxKsyunz <[email protected]>

Use Spring to instantiate dsl in OpenSearchTestBase.

bc39d43

Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz requested review from Yury-Fridlyand and acarbonetto November 4, 2022 17:05

GumpacG approved these changes Nov 4, 2022

View reviewed changes

Yury-Fridlyand reviewed Nov 7, 2022

View reviewed changes

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

Yury-Fridlyand reviewed Nov 7, 2022

View reviewed changes

Yury-Fridlyand approved these changes Nov 7, 2022

View reviewed changes

Address CR feedback.

5bed983

Add FunctionDSL.nullMissingHandlingWithProperties to all for consistent null and missing value handling across all functions. Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz force-pushed the dev-constant-now branch from 527a735 to 5bed983 Compare November 8, 2022 01:07

acarbonetto approved these changes Nov 8, 2022

View reviewed changes

Yury-Fridlyand approved these changes Nov 8, 2022

View reviewed changes

MaxKsyunz merged commit 6614306 into integ-constant-now Nov 8, 2022

MaxKsyunz deleted the dev-constant-now branch February 8, 2023 17:59

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

Updated release notes to add the version increment PR in the release …

7cdaf8d

…notes for 2.3.0 version. (#149) Signed-off-by: Navneet Verma <[email protected]> Signed-off-by: Navneet Verma <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use query execution start time as the value of now-like functions. #149

Use query execution start time as the value of now-like functions. #149

MaxKsyunz commented Oct 31, 2022

Yury-Fridlyand commented Oct 31, 2022

Yury-Fridlyand left a comment

codecov bot commented Nov 1, 2022 •

edited

Loading

Yury-Fridlyand left a comment

Yury-Fridlyand Nov 2, 2022

Yury-Fridlyand Nov 2, 2022

Yury-Fridlyand Nov 2, 2022

Yury-Fridlyand Nov 2, 2022

MaxKsyunz Nov 5, 2022

Yury-Fridlyand commented Nov 4, 2022

MaxKsyunz commented Nov 5, 2022

Yury-Fridlyand commented Nov 7, 2022

Yury-Fridlyand Nov 7, 2022

MaxKsyunz Nov 7, 2022

Yury-Fridlyand Nov 7, 2022

Yury-Fridlyand Nov 7, 2022

MaxKsyunz commented Nov 7, 2022

MitchellGale commented Nov 10, 2022

Use query execution start time as the value of now-like functions. #149

Use query execution start time as the value of now-like functions. #149

Conversation

MaxKsyunz commented Oct 31, 2022

Description

Issues Resolved

Check List

Yury-Fridlyand commented Oct 31, 2022

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2022 • edited Loading

Codecov Report

Yury-Fridlyand 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

Yury-Fridlyand commented Nov 4, 2022

MaxKsyunz commented Nov 5, 2022

Yury-Fridlyand commented Nov 7, 2022

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

MaxKsyunz commented Nov 7, 2022

MitchellGale commented Nov 10, 2022

codecov bot commented Nov 1, 2022 •

edited

Loading