-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(spark): add trunc, date_trunc and time_trunc functions #19829
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| Ok(ExprSimplifyResult::Simplified(Expr::ScalarFunction( | ||
| ScalarFunction::new_udf( | ||
| datafusion_functions::datetime::date_trunc(), |
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.
Just concerned about if matching return field nullability here is something we should watch 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.
yep DF's date_trunc returm field will be nullable.. if #19511 goes through it should fix this issue
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 don't see #19511 landing anytime soon so we might need to fix this in the DF date_trunc to ensure consistency here
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.
done
datafusion/sqllogictest/test_files/spark/datetime/date_trunc.slt
Outdated
Show resolved
Hide resolved
datafusion/sqllogictest/test_files/spark/datetime/time_trunc.slt
Outdated
Show resolved
Hide resolved
| args: Vec<Expr>, | ||
| _info: &SimplifyContext, | ||
| ) -> Result<ExprSimplifyResult> { | ||
| let fmt_expr = &args[0]; |
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.
| let fmt_expr = &args[0]; | |
| let [fmt_expr, time_expr] = take_function_args(self.name(), args)?; |
| } | ||
|
|
||
| Ok(ExprSimplifyResult::Simplified(Expr::ScalarFunction( | ||
| ScalarFunction::new_udf(datafusion_functions::datetime::date_trunc(), args), |
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.
fmt is normalized (lowercased) above and validated but here you pass the original args (non-normalized).
Maybe it will be better to pass the fmt:
let fmt_expr = Expr::Literal(ScalarValue::new_utf8(fmt.as_str()), None);
...
ScalarFunction::new_udf(
datafusion_functions::datetime::date_trunc(),
vec![fmt_expr, time_expr],
),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.
shouldn't matter, DF will handle the original argument as well and lowercase it
|
@cht42 could you add tests for DST handling with different time zones? I am not convinced that the current PR handles this correctly. Here is an AI-generated test that highlights the issue: Spark repro: $ /opt/spark-3.5.7-bin-hadoop3/bin/spark-sql --conf spark.sql.session.timeZone=America/New_York -e "SELECT date_trunc('DAY', timestamp'2024-07-15T03:30:00Z');"
...
Spark master: local[*], Application Id: local-1768499563499
2024-07-14 00:00:00 |
|
good catch @andygrove , I added the tests and some logic to handle those cases. let me know what you think |
| &self.signature | ||
| } | ||
|
|
||
| // keep return_type implementation for information schema generation |
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 seems like a bug
| // Timestamp without timezone: use as-is | ||
| _ => ts_expr, |
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.
The above arm catches all timestamps though? Since it doesn't match on Some(tz); so when would this arm take effect?
Looks good. Thanks for addressing that. |
Which issue does this PR close?
datafusion-sparkSpark Compatible Functions #15914Rationale for this change
implement spark:
What changes are included in this PR?
Add spark compatible wrappers around datafusion date_trunc function to handle spark specificities.
Are these changes tested?
Yes in SLT
Are there any user-facing changes?
Yes