Repository navigation
Conversation
Spark's date_part('SECOND', ts) / EXTRACT(SECOND FROM ts) return
DECIMAL(8, 6) with the microsecond fraction. Rewrite those calls to a
dedicated seconds UDF at plan time so the return type is fixed before
analysis, and keep standalone second(ts) as Int32.
harsh-ande
left a comment
There was a problem hiding this comment.
Thanks @ump45nose, returning Spark's DECIMAL(8, 6) with the fractional seconds is a good fix. Added couple of minor comments.
| impl ScalarUDFImpl for SparkDatePartSeconds { | ||
| fn name(&self) -> &str { | ||
| // Keep the display name as date_part so EXPLAIN/DESCRIBE stay familiar. | ||
| "date_part" |
There was a problem hiding this comment.
I found one regression in this PR: the new seconds UDF is also named date_part but it takes only the timestamp argument. As a result the default column name is now date_part(t.ts) instead of date_part(Utf8("SECOND"),t.ts). If we have 2 seconds aliases in the same SELECT, they now collide. Example:
SELECT date_part('SECOND', ts), date_part('S', ts)
FROM (VALUES (timestamp '2020-01-01 00:00:01.5')) AS t(ts);
This runs on main but fails with the following error in this branch - Projections require unique expression names but the expression "date_part(t.ts)" at position 0 and "date_part(t.ts)" at position 1 have the same name.
Could the rewritten call keep the part literal as its first argument, and ignored at runtime, so the expression name stays the same as before? An slt case with this query would guard against it further regressing.
| && is_seconds_part(&part) | ||
| { | ||
| let date_expr = args.into_iter().nth(1).ok_or_else(|| { | ||
| datafusion_common::DataFusionError::Internal( |
There was a problem hiding this comment.
Instead of building DataFusionError::Internal(..) directly, you can use internal_datafusion_err!(..).
| && is_seconds_part(&part) | ||
| { | ||
| let date_expr = expr.args.into_iter().nth(1).ok_or_else(|| { | ||
| datafusion_common::DataFusionError::Internal( |
|
Closing due to
It's impossible to contribute responsibly in such PR volume, please don't do automated contribution in this project. @harsh-ande I think it's possible that this PR is already working as is, the issue is that it's harmful to encourage fully automated contributions. If you could verify this PR's direction valid, let's open a replacement PR, would appreciate it 🙏🏼 |
Which issue does this PR close?
Rationale for this change
In Spark,
date_part('SECOND', ts)andEXTRACT(SECOND FROM ts)returnDECIMAL(8, 6)that keeps the microsecond fraction (for example45.123456). DataFusion's Sparkdate_partalways returnedInt32and rewrote seconds to the core integerdate_part, so the fraction was dropped. The standalonesecond(ts)function correctly staysINTand is unchanged.What changes are included in this PR?
date_part/datepart/EXTRACTfor the SECOND aliases (SECOND,S,SEC,SECONDS,SECS) onto a dedicated seconds UDF that returnsDecimal128(8, 6)assecond + fraction.SparkFunctionPlannerso the return type is fixed before analysis (a user-written'SECOND'::stringcast hides the literal fromreturn_field_from_args).arrow_typeof,DESCRIBE, andEXTRACT(SECOND …).What is the testing strategy for this PR?
cargo test -p datafusion-sqllogictest --test sqllogictests -- spark/datetime/date_partcargo test -p datafusion-sqllogictest --test sqllogictests -- spark/datetime/second(standalonesecondstill Int32)cargo test -p datafusion-spark --libAre there any user-facing changes?
Yes for the Spark dialect:
date_part('SECOND', …)/EXTRACT(SECOND FROM …)now returnDECIMAL(8, 6)with fractional seconds, matching Spark. Standalonesecond(ts)is unchanged.AI assistance
Used an AI coding assistant for the implementation and tests; I reviewed the change and wrote this description myself.