-
Notifications
You must be signed in to change notification settings - Fork 0
Add implementation of now, sysdate, localtime and similar functions
#92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| import java.math.BigDecimal; | ||
| import java.math.RoundingMode; | ||
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
| import java.time.ZoneId; | ||
| import java.time.ZoneOffset; | ||
| import java.time.format.TextStyle; | ||
| import java.time.temporal.ChronoUnit; | ||
| import java.util.Locale; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.TimeUnit; | ||
| import lombok.experimental.UtilityClass; | ||
| import org.opensearch.sql.common.utils.LogUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused imports
| UTC_DATE(FunctionName.of("UTC_DATE")), | ||
| UTC_TIME(FunctionName.of("UTC_TIME")), | ||
| UTC_TIMESTAMP(FunctionName.of("UTC_TIMESTAMP")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix case
| private FunctionResolver utc_timestamp() { | ||
| return define(BuiltinFunctionName.UTC_TIMESTAMP.getName(), | ||
| impl(() -> new ExprDatetimeValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalDateTime()), DATETIME), | ||
| impl((v) -> new ExprDatetimeValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalDateTime()), DATETIME, INTEGER) | ||
| ); | ||
| } | ||
|
|
||
| private FunctionResolver utc_time() { | ||
| return define(BuiltinFunctionName.UTC_TIME.getName(), | ||
| impl(() -> new ExprTimeValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalTime()), TIME), | ||
| impl((v) -> new ExprTimeValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalTime()), TIME, INTEGER) | ||
| ); | ||
| } | ||
|
|
||
| private FunctionResolver utc_date() { | ||
| return define(BuiltinFunctionName.UTC_DATE.getName(), | ||
| impl(() -> new ExprDateValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalDate()), DATE), | ||
| impl((v) -> new ExprDateValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalDate()), DATE, INTEGER) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix UTC functions, they return local time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these functions are covered by other stories. Maybe you should wait to add them?
| return requestId; | ||
| } | ||
|
|
||
| public static void recordProcessingStarted() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogUtils is not appropriate container for this data.
A better approach is capture time when SQLService is created.
org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java:99 is a good starting point.
cc @acarbonetto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogUtils was chosen because it currently houses the requestId, which shares a similar responsibility.
I was going to suggest that we refactor all of this code (and remove the LogUtils class)
We want to choose the earliest available moment to create the start time, but I would have assumed SQLService is created when at application startup, not for each request. But then, I haven't looked at the class yet.
| return String.format("%s %s", DateTimeFormatter.ISO_DATE.format(datetime), | ||
| DateTimeFormatter.ISO_TIME.format((datetime.getNano() == 0) | ||
| ? datetime.truncatedTo(ChronoUnit.SECONDS) : datetime)); | ||
| return datetime.format(FORMATTER_ZERO_PAD_NANOS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the new datetime formatter same as old formatter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not.
@Yury-Fridlyand we do need to consider backwards compatibility too. Make sure we aren't breaking anything 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as ExprTimeValue formatter.
| @Override | ||
| public String value() { | ||
| return DateTimeFormatter.ISO_LOCAL_TIME.format(time); | ||
| return time.format(FORMATTER_ZERO_PAD_NANOS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the new formatter produces different results from the old.
Is that correct? If so, why change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was an experiment. Makes second fraction always printed with precision 6 digits with zero padding.
It could be helpful for cases when user wants to parse DateTIme.
| * NOW() returns a constant time that indicates the time at which the statement began to execute | ||
| * @return ExprValue that contains LocalDateTime object | ||
| */ | ||
| private ExprValue exprNow(Optional<Integer> fsp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does fsp stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fractional seconds precision https://database.guide/now-examples-mysql/ - But we shouldn't assume the user knows this.
Please add fsp as a @param in the javadoc
| } | ||
|
|
||
| private Function visitFunction(String functionName, FunctionArgsContext args) { | ||
| if (args == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change this to one return statement and use ?: to construct the funcArgs argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Copied from SQL AST builder, so I will update both.
| return String.format("%s %s", DateTimeFormatter.ISO_DATE.format(datetime), | ||
| DateTimeFormatter.ISO_TIME.format((datetime.getNano() == 0) | ||
| ? datetime.truncatedTo(ChronoUnit.SECONDS) : datetime)); | ||
| return datetime.format(FORMATTER_ZERO_PAD_NANOS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not.
@Yury-Fridlyand we do need to consider backwards compatibility too. Make sure we aren't breaking anything 😬
| private static final DateTimeFormatter FORMATTER_ZERO_PAD_NANOS; | ||
| private static final int MIN_FRACTION_SECONDS = 0; | ||
| private static final int MAX_FRACTION_SECONDS = 9; | ||
| private static final int FRACTION_SECONDS_TO_PRINT = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow naming conventions (or fix the other conventions if necessary. I think we should call this: MAX_FRACTION_SECONDS_TO_PRINT
core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
Outdated
Show resolved
Hide resolved
| private FunctionResolver utc_timestamp() { | ||
| return define(BuiltinFunctionName.UTC_TIMESTAMP.getName(), | ||
| impl(() -> new ExprDatetimeValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalDateTime()), DATETIME), | ||
| impl((v) -> new ExprDatetimeValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalDateTime()), DATETIME, INTEGER) | ||
| ); | ||
| } | ||
|
|
||
| private FunctionResolver utc_time() { | ||
| return define(BuiltinFunctionName.UTC_TIME.getName(), | ||
| impl(() -> new ExprTimeValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalTime()), TIME), | ||
| impl((v) -> new ExprTimeValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalTime()), TIME, INTEGER) | ||
| ); | ||
| } | ||
|
|
||
| private FunctionResolver utc_date() { | ||
| return define(BuiltinFunctionName.UTC_DATE.getName(), | ||
| impl(() -> new ExprDateValue(sysDate(Optional.empty()).atZone(ZoneId.of("UTC")).toLocalDate()), DATE), | ||
| impl((v) -> new ExprDateValue(sysDate(Optional.of(v.integerValue())).atZone(ZoneId.of("UTC")).toLocalDate()), DATE, INTEGER) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these functions are covered by other stories. Maybe you should wait to add them?
| * NOW() returns a constant time that indicates the time at which the statement began to execute | ||
| * @return ExprValue that contains LocalDateTime object | ||
| */ | ||
| private ExprValue exprNow(Optional<Integer> fsp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fractional seconds precision https://database.guide/now-examples-mysql/ - But we shouldn't assume the user knows this.
Please add fsp as a @param in the javadoc
| var res = LogUtils.getProcessingStartedTime(); | ||
| if (fsp.isEmpty()) | ||
| return new ExprDatetimeValue(res); | ||
| var default_precision = 9; // There are 10^9 nanoseconds in one second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic number 9
| if (fsp.isEmpty()) | ||
| return new ExprDatetimeValue(res); | ||
| var default_precision = 9; // There are 10^9 nanoseconds in one second | ||
| if (fsp.get() < 0 || fsp.get() > 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more magic numbers
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * NOW() returns a constant time that indicates the time at which the statement began to execute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement or query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from MySQL doc: https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_now.
acarbonetto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| @ParameterizedTest(name = "{1}") | ||
| @MethodSource("functionNames") | ||
| public void the_test(Function<Expression[], FunctionExpression> function, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to a more expressive name rather than "the_test"
now, sysdate, localtime and similar functionsnow, sysdate, localtime and similar functions
Codecov Report
@@ Coverage Diff @@
## integ-datetime-now #92 +/- ##
========================================================
+ Coverage 94.78% 94.87% +0.08%
- Complexity 2880 2936 +56
========================================================
Files 286 287 +1
Lines 7735 7859 +124
Branches 565 572 +7
========================================================
+ Hits 7332 7456 +124
Misses 349 349
Partials 54 54
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| run: ./gradlew build assemble | ||
|
|
||
| - name: Run backward compatibility tests | ||
| run: ./scripts/bwctest.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR is showing commits from other work. I'd suggest that you rebase your branch to remove those commits.
…ions. Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
…LogUtils`. Optimize `now` and `sysdate` calls. Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
bfc7477 to
cb2efbe
Compare
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
|
Waiting for |
Signed-off-by: Yury Fridlyand <[email protected]>
8272c93 to
a7b2080
Compare
…ions (#92) Signed-off-by: Yury Fridlyand <[email protected]>
…ions (#92) Signed-off-by: Yury-Fridlyand <[email protected]>
…ions (#92) 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]>
Signed-off-by: Yury Fridlyand [email protected]
Updated, please re-read
Description
New functions added:
New signatures:
now():datetimenow(int):datetimecurrent_timestamp():datetimecurrent_timestamp(int):datetimecurrent_timestamp:datetimelocaltimestamp():datetimelocaltimestamp(int):datetimelocaltimestamp:datetimelocaltime():datetimelocaltime(int):datetimelocaltime:datetimesysdate():datetimesysdate(int):datetimecurtime():timecurtime(int):timecurrent_time():timecurrent_time:timecurrent_time(int):timecurdate():datecurrent_date():datecurrent_date:dateIf the argument is given it specifies a fractional seconds precision from 0 to 6, the return value includes a fractional seconds part of that many digits. Default precision: 6.
Code updated to always fill padding by zeroes up to 6 to simplify further parsing. Note: double values have no pad filling and might be rounded.
Added automatic type cast rules:datetime->doubletime->doubledate->intSo all functions could be used in math operations likenow() + 0UPD
Reverted, because this affected other functionality.
Tests
SQL
PPL
Implementation details
NOWandSYSDATEdifferenceAccording to
MySQLstandardI had to save and then to pick time when plugin receives a query.
LogUtilswas chosen to store this unless a better candidate introduced.TwoLogUtilsThere are
LogUtilsclasses that do the same:common/src/main/java/org/opensearch/sql/common/utils/LogUtils.javalegacy/src/main/java/org/opensearch/sql/legacy/utils/LogUtils.javaFirst one is used in
PPLentry point and accessible incore.Second one is used in
SQLentry point and not available incore, becausecoredepends onlegacy. Adding access tolegacy/.../LogUtilsincorecreates a circular dependency.I had to duplicate code in both
LogUtilsuntil duplicate classes are removed in scope of another task.UPD
common...LogUtilswas renamed toQueryContext.legacy...LogUtilswas deleted. All its call were redirected toQueryContext.Automatic cast
These changes include
ExprCoreTypeExprCoreTypeTypeCastOperatorLuceneQuery**) Kind of duplicated code, no idea where it is used
See also
WideningTypeRule.javaandFunctionResolver.java.UPD
Reverted in 9f6b66c.
Parser update
datetimeConstantLiteralwas added toSQLandPPLparsers. It lists pseudo-constants which actually invoke corresponding functions without(). SoCURRENT_DATEis shortcut forCURRENT_DATE()and so on.AstExpressionBuilderwere updated to call a function whendatetimeConstantLiteralmet.AstExpressionBuilderinPPLis updated a bit by addingvisitFunctionmethod and removing some duplicated code. Copied fromAstExpressionBuilderinSQL.ANTLRrules were copied fromSQLtoPPL, soPPLgotdateLiteral,timeLiteralandtimestampLiteralwhich are not used yet.Zero paddingExprDatetimeValueandExprTmeValueare updated to fill blank space by zeroes when serialized intojson.Could be reverted if found odd.
UPD
Reverted.
Doctests
Doctests for new functions always fail, because test validate exact string match for result. I added docs, but disabled doctests for them.
See also
Issues Resolved
opensearch-project#46
opensearch-project#260
TODOs
Check List
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.