Rework on `now` function implementation #113

Yury-Fridlyand · 2022-09-03T02:10:08Z

Description

Address opensearch-project#754 feedback link

Issues Resolved

now function value is calculated on AST analysis stage in ExpressionAnalyzer and value is cached in AnalysisContext. This happens with all synonyms of now too.

Limitations

Value of now was calculated in 2 stages: (1) get value, (2) format it. ExpressionAnalyzer is not capable of caching intermediate value and it shouldn't be overloaded by now function logic, so I have to remove formatting stage. now (and all synonyms) don't accept anymore fsp argument which describes formatting. Otherwise AnalysisContext stores different resulting values for now(), now(6) and now(0), which are different in scope of a query - that is incorrect.

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: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2022-09-03T02:11:25Z

@penghuo, please, have a look on the proposed changes. If everything is OK, it will be merged into PR opensearch-project#754's branch and update it.

MitchellGale · 2022-09-06T15:58:00Z

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

-        impl(() -> new ExprDatetimeValue(now((Integer)null)), DATETIME),
-        impl((v) -> new ExprDatetimeValue(now(v.integerValue())), DATETIME, INTEGER)
+        impl(() -> new ExprDatetimeValue(formatNow(null)), DATETIME)
+    //impl((v) -> new ExprDatetimeValue(formatNow(v.integerValue())), DATETIME, INTEGER)


Should remove the comment.

MitchellGale · 2022-09-06T16:00:03Z

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

-        impl(() -> new ExprTimeValue(sysDate(null).toLocalTime()), TIME),
-        impl((v) -> new ExprTimeValue(sysDate(v.integerValue()).toLocalTime()), TIME, INTEGER)
+        impl(() -> new ExprTimeValue(formatNow(null).toLocalTime()), TIME)
+    //impl((v) -> new ExprTimeValue(formatNow(v.integerValue()).toLocalTime()), TIME, INTEGER)


Should probably remove the comment.

MitchellGale · 2022-09-06T16:04:28Z

docs/user/dql/functions.rst

-    +----------------------------+---------------------+
+    +----------------------------+----------------------------+
+    | value_1                    | value_2                    |
+    |----------------------------+----------------------------+


Should be a vertical pipe (|) at the end of this line instead of +

Oh right! We don't have doctests running for these functions to validate.

MitchellGale · 2022-09-06T16:06:03Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

-    | TIMESTAMP | TO_DAYS | YEAR | WEEK | DATE_FORMAT | NOW | CURDATE | CURRENT_DATE | CURTIME | CURRENT_TIME
-    | LOCALTIME | CURRENT_TIMESTAMP | LOCALTIMESTAMP | SYSDATE | UTC_TIMESTAMP | UTC_DATE | UTC_TIME
-    | MAKETIME | MAKEDATE
+    | TIMESTAMP | TO_DAYS | YEAR | WEEK | DATE_FORMAT | SYSDATE | MAKETIME | MAKEDATE


Please add these functions in alphabetical order to remain consistent.

MitchellGale · 2022-09-06T16:23:54Z

sql/src/main/antlr/OpenSearchSQLParser.g4

-    | TIMESTAMP | TO_DAYS | YEAR | WEEK | DATE_FORMAT | NOW | CURDATE | CURRENT_DATE | CURTIME | CURRENT_TIME
-    | LOCALTIME | CURRENT_TIMESTAMP | LOCALTIMESTAMP | SYSDATE | UTC_TIMESTAMP | UTC_DATE | UTC_TIME
-    | MAKETIME | MAKEDATE
+    | TIMESTAMP | TO_DAYS | YEAR | WEEK | DATE_FORMAT | SYSDATE | MAKETIME | MAKEDATE


Please put in alphabetical order.

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

acarbonetto · 2022-09-08T16:35:48Z

core/src/main/java/org/opensearch/sql/analysis/AnalysisContext.java

+  /**
+   * Storage for values of functions which return a constant value.
+   * We are storing the values there to use it in sequential calls to those functions.
+   * For example, `now` function should the same value during processing a query.


"Constant" suggests that the return value is hard-coded.
I would re-word this as a cache for function return values.
Maybe something like functionToExpressionCache.
Note: It would be more useful if we could include arguments in the key, then we could use this cache to calculate and cache function calls with different arguments.

I'm open to rename it.

Regarding arguments - they are already included, but now function (and all synonyms) doesn't accept argument anymore. Otherwise there is bug, when now() and now(6) have different values, because were calculated twice, because they can't cross-use cached value.

Yes. No arguments on now is fine.

Renaming done in ce2e8bf.

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

core/src/main/java/org/opensearch/sql/analysis/AnalysisContext.java

MaxKsyunz · 2022-09-12T23:49:38Z

core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java

+  @Override
+  public Expression visitFunctionWithCachedValue(FunctionWithCachedValue node,
+                                                 AnalysisContext context) {
+    var valueName = node.toString();


I think it's sufficient to use node.getFuncName here.

toString is not a great option to generate a hashmap key. All it guarantees is that the result is a string -- intended for logging and debugging. There is no requirement that it's unique for different object.

In the case of Function object, it happens to include name and parameters making it ok as a key but it would be just as valid for Function.toString to return "An unresolved function expression" or "Function object 0xDEADBEEF"

Thank you, fixed in 4008c75.

MaxKsyunz · 2022-09-12T23:51:28Z

core/src/main/java/org/opensearch/sql/ast/expression/FunctionWithCachedValue.java

+ * [1] Constant at execution time.
+ */
+@EqualsAndHashCode(callSuper = false)
+public class FunctionWithCachedValue extends Function {


These are called constant functions in math.
ConstantFunction or ConstantValueFunction better explain what's special in this case.

Thank you, renamed in 6ef778d.

MaxKsyunz · 2022-09-12T23:55:17Z

sql/src/main/antlr/OpenSearchSQLParser.g4

@@ -307,6 +307,11 @@ functionCall
    | aggregateFunction (orderByClause)? filterClause               #filteredAggregationFunctionCall
    | relevanceFunction                                             #relevanceFunctionCall
    | highlightFunction                                             #highlightFunctionCall
+    | functionWithCachedValue                                       #functionLikeConstantCall


It'd be cleaner if #functionLikeConstantCall matched the rule name.

Thank you, renamed in 6ef778d.

MaxKsyunz · 2022-09-12T23:55:23Z

sql/src/main/antlr/OpenSearchSQLParser.g4

+    | functionWithCachedValue                                       #functionLikeConstantCall
+    ;
+
+functionWithCachedValue


constantValuedFunction or constantFunction are more appropriate names for this rule.

Thank you, renamed in 6ef778d.

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

…hedValue` to `ConstantFunction`. Signed-off-by: Yury-Fridlyand <[email protected]>

MitchellGale · 2022-09-13T08:03:27Z

Unexpected result for now with number up to 6. Unsure what the standard is for warning messages for invalid inputs, but could also give a warning for values too high.

opensearchsql> select now(1);                                                                                                                                                                                                                               
{'reason': 'Invalid SQL query', 'details': 'now function expected {[]}, but get [INTEGER]', 'type': 'ExpressionEvaluationException'}

Invalid argument on now gives warning in MySQL. Should OpenSearchSQL match?

mysql> SELECT now(10);
ERROR 1426 (42000): Too-big precision 10 specified for 'now'. Maximum is 6.

MitchellGale · 2022-09-13T08:06:09Z

Results differ between current_timestamp in MySQL and OpenSearchSQL.

OpenSearchSQL:

opensearchsql> select current_timestamp();                                                                                                                                                                                                                  
fetched rows / total rows = 1/1
+----------------------------+
| current_timestamp()        |
|----------------------------|
| 2022-09-13 01:04:13.512658 |
+----------------------------+

MySQL:

+---------------------+
| current_timestamp() |
+---------------------+
| 2022-09-13 08:04:30 |
+---------------------+
1 row in set (0.00 sec)

Results differ between localtimestamp in MySQL and OpenSearchSQL.

OpenSearchSQL:

opensearchsql> select localtimestamp();                                                                                                                                                                                                                     
fetched rows / total rows = 1/1
+----------------------------+
| localtimestamp()           |
|----------------------------|
| 2022-09-13 01:06:39.647093 |
+----------------------------+

MySQL:

mysql> select localtimestamp();
+---------------------+
| localtimestamp()    |
+---------------------+
| 2022-09-13 08:07:07 |
+---------------------+
1 row in set (0.00 sec)

Yury-Fridlyand · 2022-09-13T16:43:32Z

Unexpected result for now with number up to 6. Unsure what the standard is for warning messages for invalid inputs, but could also give a warning for values too high.

opensearchsql> select now(1);                                                                                                                                                                                                                               
{'reason': 'Invalid SQL query', 'details': 'now function expected {[]}, but get [INTEGER]', 'type': 'ExpressionEvaluationException'}

New implementation creates a cache entry for function and its arguments. There is no common cache for now() and now(6) calls - function executed twice which is completely incorrect.
I removed now(integer) signature, you can see it in DateTimeFunction.java, lines 108-115.

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

Yury-Fridlyand · 2022-09-13T16:57:07Z

Results differ between current_timestamp in MySQL and OpenSearchSQL.

Nice catch! Fixed in 64ccf47.

MitchellGale · 2022-09-14T17:41:13Z

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

    if (fsp == null) {
-      return res;
+      fsp = 0;
    }
    var defaultPrecision = 9; // There are 10^9 nanoseconds in one second
    if (fsp < 0 || fsp > 6) { // Check that the argument is in the allowed range [0, 6]


Do you need this if you are not having the values in now be handled?

sysdate() uses the same function under the hood and it has an argument.
Try

select sysdate(), sysdate(0), sysdate(6);

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

…ions (opensearch-project#754) * Add implementation of `now`, `sysdate`, `localtime` and similar functions (#92) Signed-off-by: Yury-Fridlyand <[email protected]> * Rework on `now` function implementation (#113) Signed-off-by: Yury-Fridlyand <[email protected]> * Minor SQL ANTLR clean-up. Signed-off-by: Yury-Fridlyand <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand added 6 commits August 30, 2022 17:38

Try to process now in ExpressionAnalyzer.

c88a5ce

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

Optimize FunctionLikeConstant.

7534d46

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

Code cleanup.

0fa818d

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

Test coverage.

eec2e19

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

Fix tests.

2ea3a09

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

Fix & update tests, style and docs.

2c3047a

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

Yury-Fridlyand requested review from forestmvey, MaxKsyunz and MitchellGale September 3, 2022 02:11

Yury-Fridlyand mentioned this pull request Sep 3, 2022

Add implementation of now, sysdate, localtime and similar functions opensearch-project/sql#754

Merged

6 tasks

MitchellGale requested changes Sep 6, 2022

View reviewed changes

MaxKsyunz requested a review from acarbonetto September 6, 2022 16:57

Yury-Fridlyand changed the title ~~Rework on now function implementation~~ Rework on now function implementation Sep 6, 2022

Address PR feedback by @MitchellGale-BitQuill.

b9542d8

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

Yury-Fridlyand requested a review from MitchellGale September 6, 2022 22:51

MaxKsyunz approved these changes Sep 7, 2022

View reviewed changes

Bit-Quill deleted a comment from codecov bot Sep 8, 2022

acarbonetto approved these changes Sep 8, 2022

View reviewed changes

Rename FunctionLikeConstant to FunctionWithCachedValue.

ce2e8bf

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

This comment was marked as spam.

Sign in to view

MaxKsyunz requested changes Sep 12, 2022

View reviewed changes

Yury-Fridlyand added 2 commits September 12, 2022 17:11

Minor cleanup.

4008c75

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

More thorough renaming of FunctionLikeConstant and `FunctionWithCac…

6ef778d

…hedValue` to `ConstantFunction`. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested a review from MaxKsyunz September 13, 2022 01:33

Change default precision to match MySQL.

64ccf47

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

MaxKsyunz approved these changes Sep 14, 2022

View reviewed changes

MitchellGale reviewed Sep 14, 2022

View reviewed changes

MitchellGale approved these changes Sep 14, 2022

View reviewed changes

Yury-Fridlyand merged commit 1e42e2b into integ-datetime-now Sep 15, 2022

Yury-Fridlyand deleted the dev-datetime-now-rework branch September 15, 2022 02:20

Yury-Fridlyand added a commit that referenced this pull request Sep 15, 2022

Rework on now function implementation (#113)

425a998

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

Yury-Fridlyand mentioned this pull request Sep 15, 2022

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

Open

Yury-Fridlyand mentioned this pull request Oct 31, 2022

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

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework on `now` function implementation #113

Rework on `now` function implementation #113

Yury-Fridlyand commented Sep 3, 2022

Yury-Fridlyand commented Sep 3, 2022

MitchellGale Sep 6, 2022

MitchellGale Sep 6, 2022

MitchellGale Sep 6, 2022

Yury-Fridlyand Sep 6, 2022

MitchellGale Sep 6, 2022

MitchellGale Sep 6, 2022

acarbonetto Sep 8, 2022

Yury-Fridlyand Sep 8, 2022

acarbonetto Sep 8, 2022

Yury-Fridlyand Sep 12, 2022

This comment was marked as spam.

MaxKsyunz Sep 12, 2022

Yury-Fridlyand Sep 13, 2022

MaxKsyunz Sep 12, 2022

Yury-Fridlyand Sep 13, 2022

MaxKsyunz Sep 12, 2022

Yury-Fridlyand Sep 13, 2022

MaxKsyunz Sep 12, 2022

Yury-Fridlyand Sep 13, 2022

MitchellGale commented Sep 13, 2022

MitchellGale commented Sep 13, 2022 •

edited

Loading

Yury-Fridlyand commented Sep 13, 2022

Yury-Fridlyand commented Sep 13, 2022

MitchellGale Sep 14, 2022

Yury-Fridlyand Sep 14, 2022

Rework on now function implementation #113

Rework on now function implementation #113

Conversation

Yury-Fridlyand commented Sep 3, 2022

Description

Issues Resolved

Limitations

Check List

Yury-Fridlyand commented Sep 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

MitchellGale commented Sep 13, 2022

MitchellGale commented Sep 13, 2022 • edited Loading

Yury-Fridlyand commented Sep 13, 2022

Yury-Fridlyand commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rework on `now` function implementation #113

Rework on `now` function implementation #113

MitchellGale commented Sep 13, 2022 •

edited

Loading