Skip to content

Add datetime functions #3473

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 224 commits into from
Apr 3, 2025
Merged

Add datetime functions #3473

merged 224 commits into from
Apr 3, 2025

Conversation

xinyual
Copy link
Contributor

@xinyual xinyual commented Mar 24, 2025 •
edited
Loading

Description

We have several tasks in this PR:

  1. Add datetime functions.
  2. Add IT for these functions
  3. Add approximate percentile functions and IT
  4. Add comparison between time/datetime/date

For this PR,
we miss

  1. Validation for argument

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
#3400

I have tested the nonFallbackITs in the #3400 locally. There are some issues:
NonFallbackCalciteNowLikeFunctionIT: pass all
NonFallbackCalciteDateTimeFunctionIT: pass all

NonFallbackCalciteDateTimeImplementationIT: pass
NonFallbackCalciteDateTimeComparisonIT: pass all
NonFallbackCalciteConvertTZFunctionIT: pass

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

xinyual and others added 30 commits February 24, 2025 10:42
@xinyual
add condition and string func
7f1eba4 
Signed-off-by: xinyual <[email protected]>
@xinyual
add atan
86f1b9c 
Signed-off-by: xinyual <[email protected]>
@yuancu
[WIP] map opensearch math functions to calcite implementations
7d46cbc 
Signed-off-by: Yuanchun Shen <[email protected]>
@xinyual
add transfer for log
ce264dd 
Signed-off-by: xinyual <[email protected]>
@xinyual
Merge branch 'mapToCalciteBuiltIn' into map2calcite
06a7b65 
Signed-off-by: Xinyuan Lu <[email protected]>
@xinyual
Merge pull request #1 from yaunchun/map2calcite
25d7d51 
Map opensearch math functions to calcite implementations
@xinyual
fix log and atan
6cc2452 
Signed-off-by: xinyual <[email protected]>
@xinyual
add udf and udaf percentile and Mod
7ff8a1c 
Signed-off-by: xinyual <[email protected]>
@xinyual
add condition udf and take agg
19e11d6 
Signed-off-by: xinyual <[email protected]>
@xinyual
add unified test framework
25867df 
Signed-off-by: xinyual <[email protected]>
@xinyual
Merge remote-tracking branch 'origin/feature/calcite-engine' into add…
65fd487 
…StringFunctions
@xinyual
fix UT/IT
7274a05 
Signed-off-by: xinyual <[email protected]>
@xinyual
merge
6ccfd97 
Signed-off-by: xinyual <[email protected]>
@xinyual
fix UT/IT
f6120dc 
Signed-off-by: xinyual <[email protected]>
@xinyual
remove useless code change
4fa915d 
Signed-off-by: xinyual <[email protected]>
@xinyual
apply spotless
99b33cd 
Signed-off-by: xinyual <[email protected]>
@xinyual
add license and move it
0b28fab 
Signed-off-by: xinyual <[email protected]>
@xinyual
remove useless change
651db98 
Signed-off-by: xinyual <[email protected]>
@xinyual
add time functions
706f9b0 
Signed-off-by: xinyual <[email protected]>
@xinyual
fix date/utc_data/timestamp
8c0dd3b 
Signed-off-by: xinyual <[email protected]>
@xinyual
add unix_timestamp(half)
d0f4f38 
Signed-off-by: xinyual <[email protected]>
@xinyual
continue to fix unixtimestamp
2f29ab7 
Signed-off-by: xinyual <[email protected]>
@xinyual
fix unixtimestamp
8fc4b39 
Signed-off-by: xinyual <[email protected]>
@xinyual
add timestamp
6eb9204 
Signed-off-by: xinyual <[email protected]>
@xinyual
fix timestamp
5b75398 
Signed-off-by: xinyual <[email protected]>
@yuancu
Implement DATEADD function
61deedf 
Signed-off-by: Yuanchun Shen <[email protected]>
@yuancu
Implement DATE_SUB function
60a2807 
Signed-off-by: Yuanchun Shen <[email protected]>
@yuancu
WIP: Implement TIMEADD
85798af
@yuancu
Implement ADDTIME
f4c08cf 
Signed-off-by: Yuanchun Shen <[email protected]>
@yuancu
Implement DAY_OF_WEEK and DAY_OF_YEAR
02be4c4 
Signed-off-by: Yuanchun Shen <[email protected]>
xinyual added 2 commits April 1, 2025 17:14
@xinyual
remove useless code
f1d1453 
Signed-off-by: xinyual <[email protected]>
@xinyual
revert change
8a88ef9 
Signed-off-by: xinyual <[email protected]>
LantaoJin
LantaoJin reviewed Apr 1, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java Outdated
@@ -118,6 +121,22 @@ public RexNode visitLiteral(Literal node, CalcitePlanContext context) {
}
}

private SpanUnit intervalUnitToSpanUnit(IntervalUnit unit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse this https://github.com/opensearch-project/sql/blob/main/core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java#L13

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuancu

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done.

...rc/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/PostprocessDateToStringFunction.java
@@ -0,0 +1,25 @@
package org.opensearch.sql.calcite.udf.datetimeUDF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright. please check all new added files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDE: settings -> Editor -> Copyright -> Copyright Profile -> +

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java Outdated
return context.relBuilder.call(op, left, right);
}

private RexNode transferCompareForDateRelated(
RexNode candidate, CalcitePlanContext context, boolean wrapper) {
if (wrapper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapper -> whetherCompareByTime

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need this DateToString call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean PostprocessDateToStringFunction?

core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java Outdated
@@ -320,11 +363,20 @@ public RexNode visitLet(Let node, CalcitePlanContext context) {

@Override
public RexNode visitFunction(Function node, CalcitePlanContext context) {
// Re-org the code structure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/udf/UserDefinedAggFunction.java Outdated
@@ -5,6 +5,9 @@

package org.opensearch.sql.calcite.udf;

/**
* TO DO. support init with constant arguments https://github.com/opensearch-project/sql/issues/3490
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO, not TO DO

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/PostprocessForUDTFunction.java Outdated
@@ -0,0 +1,37 @@
package org.opensearch.sql.calcite.udf.datetimeUDF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java Outdated
@@ -35,10 +97,15 @@
import org.opensearch.sql.calcite.udf.textUDF.LocateFunction;
import org.opensearch.sql.calcite.udf.textUDF.ReplaceFunction;

/** TO DO: We need to refactor code to make all */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/utils/BuiltinFunctionUtils.java Outdated
@@ -204,6 +331,82 @@ static SqlOperator translate(String op) {
return TransferUserDefinedFunction(
TypeOfFunction.class, "typeof", ReturnTypes.VARCHAR_2000_NULLABLE);
// TODO Add more, ref RexImpTable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this line before default:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateFunction.java Outdated
Comment on lines 34 to 39
// try {
// return exprDate(transferInputToExprTimestampValue(args[0], (SqlTypeName) args[1], restored))
// .valueForCalcite();
// } catch (SemanticCheckException e) {
// return null;
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/TimeAddSubFunction.java Outdated
Comment on lines 42 to 47
// Instant base = InstantUtils.convertToInstant(argBase, baseType, false);
// Instant interval = InstantUtils.convertToInstant(argInterval, argIntervalType, false);
// LocalTime time = interval.atZone(ZoneOffset.UTC).toLocalTime();
// Duration duration = Duration.between(LocalTime.MIN, time);

// Instant newInstant = DateTimeApplyUtils.applyInterval(base, duration, isAdd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

qianheng-aws
qianheng-aws reviewed Apr 1, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateAddSubFunction.java Outdated
boolean isAdd = (Boolean) args[4];
SqlTypeName returnSqlType = (SqlTypeName) args[5];
ExprValue base = transferInputToExprValue(argBase, sqlTypeName);
// Instant base = InstantUtils.convertToInstant(argBase, sqlTypeName, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateAddSubFunction.java Outdated
return null;
}

if (UserDefinedFunctionUtils.containsNull(args)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already remove it.

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateAddSubFunction.java
TimeUnit unit = (TimeUnit) args[0];
long interval = ((Number) args[1]).longValue();
Object argBase = args[2];
SqlTypeName sqlTypeName = (SqlTypeName) args[3];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using SqlTypeName here? I think sql type is a subset of all types we use and date, time, timestamp of Sql type is no longer used by us after introducing UDT

Copy link
Contributor Author

@xinyual xinyual Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our current UDF, all time/timestamp/time input would be the UDT. So we directly transfer the type to corresponding sqltypetime that we can call fromObjectValue easily. See function transferDateRelatedTimeName

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These UDT's real SqlType is VARCHAR, the more appropriate way is pass in the RelDataType here and get the correct exprtype by using ExprUDT.getExprCoreType.

It works here because we have a incorrect mapping in convertSqlTypeNameToExprType(the mapping of time,timestamp,date will never be used since they are blocked now by the previous logic of handling UDT), and you pass in SqlTypeName.TIMESTAMP directly from the place where you call this funciton.

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateAddSubFunction.java
return opBinding -> {
RelDataType operandType0 = opBinding.getOperandType(6);
SqlTypeName typeName = operandType0.getSqlTypeName();
if (typeName == SqlTypeName.TIMESTAMP) {
Copy link
Collaborator

@qianheng-aws qianheng-aws Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case will we use SqlTypeName.TIMESTAMP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

core/src/main/java/org/opensearch/sql/calcite/udf/datetimeUDF/DateAddSubFunction.java
SqlTypeName returnSqlType = (SqlTypeName) args[5];
ExprValue base = transferInputToExprValue(argBase, sqlTypeName);
// Instant base = InstantUtils.convertToInstant(argBase, sqlTypeName, false);
FunctionProperties restored = restoreFunctionProperties(args[args.length - 1]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This properties should be set when constructing the function and I think it supports pass in FunctionProperties directly, please add a TODO here to make change in the future.

Copy link
Contributor Author

@xinyual xinyual Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Add in function restoreFunctionProperties

qianheng-aws
qianheng-aws reviewed Apr 2, 2025
View reviewed changes
...t/java/org/opensearch/sql/calcite/standalone/CalcitePPLBuiltinDatetimeFunctionInvalidIT.java Outdated

@Test
public void testYearWeekInvalid() {
assertThrows(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add assert for error message as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already add.

LantaoJin
LantaoJin reviewed Apr 2, 2025
View reviewed changes
...src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLDateTimeBuiltinFunctionIT.java Outdated
@@ -949,6 +949,7 @@ public void testMakeDateWithNullIO() {
* date value given the day number N. DATETIME: (TIMESTAMP, STRING) -> TIMESTAMP (TIMESTAMP) ->
* TIMESTAMP Converts the datetime to a new timezone
*/
@Ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this test flaky?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fix the bug and enable IT.

LantaoJin
LantaoJin previously approved these changes Apr 2, 2025
View reviewed changes
yuancu and others added 2 commits April 3, 2025 10:21
@yuancu
Reuse PlanUtils.intervalUnitToSpanUnit
d510c84 
Signed-off-by: Yuanchun Shen <[email protected]>
@xinyual
Merge pull request #24 from yuancu/fix-datetime
9d8e849 
Reuse PlanUtils.intervalUnitToSpanUnit
LantaoJin
LantaoJin approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@LantaoJin LantaoJin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @xinyual and @yuancu

@LantaoJin LantaoJin merged commit 920f711 into opensearch-project:main Apr 3, 2025
21 checks passed
@yuancu yuancu mentioned this pull request Apr 3, 2025
Draft
8 tasks
@noCharger noCharger mentioned this pull request Apr 9, 2025
Merged
2 tasks
This was referenced Apr 11, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LantaoJin LantaoJin LantaoJin approved these changes

@qianheng-aws qianheng-aws qianheng-aws approved these changes

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

Assignees
No one assigned
Labels
calcite calcite migration releated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xinyual @LantaoJin @qianheng-aws @yuancu @songkant-aws