Support to pass dynamic values to timestamp Extract function #15586
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| testHelper.testExpressionString( | ||
| new TimeExtractOperatorConversion().calciteOperator(), | ||
| ImmutableList.of( | ||
| testHelper.makeInputRef("t"), | ||
| testHelper.makeLiteral("DAY"), | ||
| testHelper.makeInputRef("timezone") | ||
| ), | ||
| makeExpression(ColumnType.LONG, "timestamp_extract(\"t\",'DAY',\"timezone\")"), | ||
| 2L | ||
| ); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
| testHelper.makeLiteral("DAY"), | ||
| testHelper.makeInputRef("timezone") | ||
| ), | ||
| makeExpression(ColumnType.LONG, "timestamp_extract(\"t\",'DAY',\"timezone\")"), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
| } | ||
|
|
||
| final Expr arg = args.get(0); |
There was a problem hiding this comment.
you could still keep it.
There was a problem hiding this comment.
no reference for this arg, so I removed it.
| if (val == null) { | ||
| // Return null if the argument if null. | ||
| return ExprEval.of(null); | ||
| } | ||
| final ISOChronology chronology = chronologyOptional.orElseGet(() -> { | ||
| Optional<String> timeZoneOptional = args.size() > 2 |
There was a problem hiding this comment.
We could avoid this check in eval and set zone to UTC outside eval itself is args size is less than or equal to 2
There was a problem hiding this comment.
I also think the usage of Optional is unnecessary here given that the usage is contained within this class. So you could also do good-ol if-null-then-x-else-y style of code.
There was a problem hiding this comment.
fixed in the latest commit. @abhishekagarwal87 Thanks for you suggestion and time
There was a problem hiding this comment.
the test error "Error while executing SQL "SELECT * FROM INFORMATION_SCHEMA.COLUMNS": Remote driver error: Unauthorized" should be irrelevant to my modification.
| final ISOChronology chronology = ISOChronology.getInstance(timeZone); | ||
|
|
||
| class TimestampExtractExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr | ||
| class TimestampExtractExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr |
There was a problem hiding this comment.
drive by comment: because eval is called every row, we often will split up implementations of Expr when one form has a dynamic argument that needs evaluated every row, see TimestampFloorExprMacro for example. This allows cases where the argument is a constant to skip some work.
There was a problem hiding this comment.
good catch and suggestion @clintropolis followed the example and fixed in the latest commit.
There was a problem hiding this comment.
@clintropolis and @abhishekagarwal87 the unit test cases("Remote driver error: Unauthorized" and "leadership changed") failed which should be not relative to my modification.
|
thank you for your contribution @AlbericByte |
Fixes #15072
Description
Before this modification , the third parameter (timezone) require to be a Literal, it will throw a error when this parameter is column Identifier.
Release note
Key changed/added classes in this PR
This PR has: