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

MaxKsyunz · 2022-11-08T20:40:14Z

Description

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. It is no longer necessary in 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]

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.

codecov-commenter · 2022-11-08T20:56:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.79%. Comparing base (e2bf254) to head (ea4588e).
Report is 406 commits behind head on 2.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #1047      +/-   ##
============================================
- Coverage     98.29%   95.79%   -2.51%     
- Complexity     3465     3470       +5     
============================================
  Files           347      358      +11     
  Lines          8647     9315     +668     
  Branches        550      668     +118     
============================================
+ Hits           8500     8923     +423     
- Misses          142      334     +192     
- Partials          5       58      +53

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ppl/src/main/antlr/OpenSearchPPLParser.g4

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

penghuo · 2022-11-09T18:20:44Z

core/src/main/java/org/opensearch/sql/expression/config/ExpressionConfig.java

@@ -51,8 +56,14 @@ public BuiltinFunctionRepository functionRepository() {
    return builtinFunctionRepository;
  }

+  @Bean


FunctionProperties should be created per-request. Currently, Spring IoC container is created per-request which is not efficient and has issue (#611) , it will be changed to per jvm (#822), then BuiltinFunctionRepository is singleton instance per jvm.

@penghuo in e7ccfc2 I moved FunctionProperties instantiation from ExpressionConfig to context creation time. Is that what you had in mind?

Not exactally.

ApplicationContext should be created per-node instead of per-request. Which means FunctionProperties should not been created in ApplicationContext.

@dai-chen also think of making FunctionRepositoy as singleton. Reference, Decouple function repository and DSL from IoC container for use anywhere #1045

I think FunctionProperties should be in a context and pass as paramater to Analyzer and FunctionProperties.

@penghuo, I will move Spring setup to SQLPlugin.createComponents and update as appropriate.

@penghuo, I will move Spring setup to SQLPlugin.createComponents and update as appropriate.

As discussed offline with @penghuo , this PR will wait on merge of #1045 into 2.x branch.

cc @dai-chen

@penghuo and @dai-chen this PR is ready for another review. The branch have been updated from 2.x and modified to work with static BuiltinFunctionRepository class.

Thanks!

- 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]>

MaxKsyunz · 2022-11-10T17:54:47Z

I'm looking into integration test failures.

@Getter

Remove FunctionProperties from ExpressionConfig and create it when create<LANG>Service is called. This ensures it's created for each query. Created FunctionPropertiesTestConfig to simplify updating unit tests to work with the change above. Removed an infrequently-used version of BuiltinFunctionRepository.compile and @Getter on BuiltinFunctionRepository.functionProperties -- it was only used for a test -- ExpressionConfigTest. Removed the test because it was testing Spring behavior and not very valuable. Signed-off-by: MaxKsyunz <[email protected]>

MaxKsyunz · 2022-11-10T21:22:56Z

I'm looking into integration test failures.

Done. There was a integration test that was instantiating PPLSerivceConfig and required a registration for FunctionProperties.

The core of reimplementation is to provide FunctionProperties at function compilation in Analyzer *or ExpressionAnalyzer). See FunctionBuilder.apply(FunctionProperties, List<Expression> arguments) AnalysisContext provides an instance of FunctionProperties. The rest of the changes are the consequence of the above. Signed-off-by: MaxKsyunz <[email protected]>

# Conflicts: # core/src/main/java/org/opensearch/sql/analysis/Analyzer.java # integ-test/src/test/java/org/opensearch/sql/ppl/StandaloneIT.java # ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java

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

They were introduced earlier before BuiltinFunctionRepository became static. Signed-off-by: MaxKsyunz <[email protected]>

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

Most functions in DSL class do not use FunctionProperties. Use a placeholder instance. Signed-off-by: MaxKsyunz <[email protected]>

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

penghuo · 2022-11-29T17:05:46Z

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

+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+public class FunctionPropertiesTestConfig {


Move TestConfig to test?

Moved in ea4588e

penghuo · 2022-11-29T17:14:03Z

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

+import java.io.Serializable;
+import java.time.Clock;
+
+public interface FunctionProperties extends Serializable {


does the interface required? i think NONE could extend FunctionProperties, and QueryFunctionProperties could be removed.

Done in ea4588e

core/src/main/java/org/opensearch/sql/expression/DSL.java

penghuo · 2022-11-29T17:22:38Z

legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java

-
-    private final boolean allowExplicitIndex;
-
-    private static final Predicate<String> CONTAINS_SUBQUERY = Pattern.compile("\\(\\s*select ").asPredicate();


could you elaborate more the change in RestSqlAction? I did not expect much change should in here.

It's a white space change IntelliJ snuck in during when merging with 2.X.

I'll redo the branch to discard those changes.

Fixed in ea4588e

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

- Change FunctionProperties into a concrete class. - Move FunctionPropertiesConfig to test since it's only used for testing. Required chaning how PPLServiceTest and StandaloneIT instantiate FunctionProperties bean. Signed-off-by: MaxKsyunz <[email protected]>

…pensearch-project#1047) Signed-off-by: Margarit Hakobyan <[email protected]>

MaxKsyunz requested a review from a team as a code owner November 8, 2022 20:40

MaxKsyunz force-pushed the integ-constant-now branch from 6614306 to 428b216 Compare November 8, 2022 23:11

Yury-Fridlyand mentioned this pull request Nov 9, 2022

Decouple function repository and DSL from IoC container for use anywhere #1045

Closed

6 tasks

Yury-Fridlyand requested review from dai-chen, joshuali925 and penghuo November 9, 2022 00:55

penghuo reviewed Nov 9, 2022

View reviewed changes

ppl/src/main/antlr/OpenSearchPPLParser.g4 Outdated Show resolved Hide resolved

penghuo reviewed Nov 9, 2022

View reviewed changes

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

penghuo reviewed Nov 9, 2022

View reviewed changes

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

penghuo reviewed Nov 9, 2022

View reviewed changes

MaxKsyunz force-pushed the integ-constant-now branch from 428b216 to 703275a Compare November 10, 2022 06:54

MaxKsyunz force-pushed the integ-constant-now branch from e7ccfc2 to e50074b Compare November 10, 2022 18:17

MitchellGale previously approved these changes Nov 11, 2022

View reviewed changes

acarbonetto previously approved these changes Nov 14, 2022

View reviewed changes

acarbonetto mentioned this pull request Nov 21, 2022

SQL ingestion and ops - standup #791

Open

MaxKsyunz dismissed stale reviews from acarbonetto and MitchellGale via f9270da November 23, 2022 22:49

MaxKsyunz added 7 commits November 23, 2022 18:06

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

9e2cb41

# Conflicts: # core/src/main/java/org/opensearch/sql/analysis/Analyzer.java # integ-test/src/test/java/org/opensearch/sql/ppl/StandaloneIT.java # ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java

Delete empty file ExpressionConfig.java

3fe6987

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

Add Temporal isClose to assert matcher for better error messages.

402dd7a

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

Simplify BuiltinFunctionRepository.resolve

8238c2a

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

Remove unnecessary springframework references.

47d0246

They were introduced earlier before BuiltinFunctionRepository became static. Signed-off-by: MaxKsyunz <[email protected]>

Remove incorrect comment

6a536fa

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

Improve description in Temporal isCloseTo

bccc771

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

MaxKsyunz added 2 commits November 23, 2022 22:26

Remove FunctionProperties parameter in BuiltinFunctionRepository.resolve

3037708

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

Introduce FunctionProperties.None to simplify DSL class.

659caaa

Most functions in DSL class do not use FunctionProperties. Use a placeholder instance. Signed-off-by: MaxKsyunz <[email protected]>

Yury-Fridlyand mentioned this pull request Nov 24, 2022

Add Week_Of_Year Function To OpenSearch Bit-Quill/opensearch-project-sql#175

Merged

6 tasks

Add unit tests for FunctionProperties.None

df65f01

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

Yury-Fridlyand mentioned this pull request Nov 25, 2022

Add Day_Of_Year Function To OpenSearch Bit-Quill/opensearch-project-sql#173

Merged

6 tasks

penghuo reviewed Nov 29, 2022

View reviewed changes

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

penghuo reviewed Nov 29, 2022

View reviewed changes

MaxKsyunz added 2 commits November 30, 2022 00:42

Merge branch '2.x' into integ-constant-now

fa8be5c

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

Address PR feedback.

ea4588e

- Change FunctionProperties into a concrete class. - Move FunctionPropertiesConfig to test since it's only used for testing. Required chaning how PPLServiceTest and StandaloneIT instantiate FunctionProperties bean. Signed-off-by: MaxKsyunz <[email protected]>

This was referenced Dec 1, 2022

Add Month_Of_Year Function To OpenSearch #1129

Merged

Add Day_Of_Year Function To OpenSearch #1128

Merged

Add Week_Of_Year Function To OpenSearch #1127

Merged

penghuo approved these changes Dec 2, 2022

View reviewed changes

MitchellGale approved these changes Dec 2, 2022

View reviewed changes

Yury-Fridlyand approved these changes Dec 2, 2022

View reviewed changes

MaxKsyunz merged commit 43ceda1 into opensearch-project:2.x Dec 2, 2022

Yury-Fridlyand deleted the integ-constant-now branch December 2, 2022 18:46

forestmvey mentioned this pull request Dec 6, 2022

Validate Field and Fields Parameters in Relevance Search Functions #1067

Merged

6 tasks

margarit-h pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Dec 6, 2022

Use query execution start time as the value of now-like functions. (o…

abe71d6

…pensearch-project#1047) Signed-off-by: Margarit Hakobyan <[email protected]>

Yury-Fridlyand mentioned this pull request Dec 8, 2022

Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. Bit-Quill/opensearch-project-sql#122

Merged

6 tasks

MitchellGale mentioned this pull request Dec 12, 2022

[BUG] DATE and TIME function may result in different times/date in one query #1157

Open

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. #1047

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

MaxKsyunz commented Nov 8, 2022

codecov-commenter commented Nov 8, 2022 •

edited by codecov bot

Loading

penghuo Nov 9, 2022

MaxKsyunz Nov 10, 2022

penghuo Nov 12, 2022 •

edited

Loading

MaxKsyunz Nov 14, 2022

MaxKsyunz Nov 14, 2022 •

edited

Loading

MaxKsyunz Nov 25, 2022

MaxKsyunz commented Nov 10, 2022

MaxKsyunz commented Nov 10, 2022

penghuo Nov 29, 2022

MaxKsyunz Nov 30, 2022

penghuo Nov 29, 2022

MaxKsyunz Nov 30, 2022

penghuo Nov 29, 2022

MaxKsyunz Nov 29, 2022

MaxKsyunz Nov 30, 2022


		private final boolean allowExplicitIndex;

		private static final Predicate<String> CONTAINS_SUBQUERY = Pattern.compile("\\(\\s*select ").asPredicate();

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

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

Conversation

MaxKsyunz commented Nov 8, 2022

Description

Unit Tests

Check List

codecov-commenter commented Nov 8, 2022 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz commented Nov 10, 2022

MaxKsyunz commented Nov 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 8, 2022 •

edited by codecov bot

Loading

penghuo Nov 12, 2022 •

edited

Loading

MaxKsyunz Nov 14, 2022 •

edited

Loading