[SPARK-53762][SQL] Add date and time conversions simplifier rule to optimizer #52493
Conversation
| */ | ||
| object SimplifyDateTimeConversions extends Rule[LogicalPlan] { | ||
| def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning( | ||
| _.containsPattern(DATETIME), ruleId) { |
There was a problem hiding this comment.
Thanks, fixed in dcf49a8.
| def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning( | ||
| _.containsPattern(DATETIME), ruleId) { | ||
| case q: LogicalPlan => q.transformExpressionsUpWithPruning( | ||
| _.containsPattern(DATETIME), ruleId) { |
There was a problem hiding this comment.
Fixed in dcf49a8.
| TimestampType, | ||
| _, | ||
| timeZoneId2, | ||
| _), |
There was a problem hiding this comment.
Just a question. Maybe, the following is better?
case DateFormatClass(
GetTimestamp(
- e @ DateFormatClass(
- _,
- pattern,
- timeZoneId),
+ e @ DateFormatClass(_, pattern, timeZoneId),
pattern2,
TimestampType,
_,There was a problem hiding this comment.
Done in dcf49a8.
| pattern2, | ||
| timeZoneId2), | ||
| pattern3, | ||
| TimestampType, |
There was a problem hiding this comment.
ditto. Maybe?
case GetTimestamp(
DateFormatClass(
- e @ GetTimestamp(
- _,
- pattern,
- TimestampType,
- _,
- timeZoneId,
- _),
+ e @ GetTimestamp(_, pattern, TimestampType, _, timeZoneId, _),
pattern2,
timeZoneId2),There was a problem hiding this comment.
Yeah, looks nicer. Fixed in dcf49a8.
| "org.apache.spark.sql.catalyst.optimizer.RewriteAsOfJoin" :: | ||
| "org.apache.spark.sql.catalyst.optimizer.SimplifyBinaryComparison" :: | ||
| "org.apache.spark.sql.catalyst.optimizer.SimplifyCaseConversionExpressions" :: | ||
| "org.apache.spark.sql.catalyst.optimizer.SimplifyDateTimeConversions" :: |
There was a problem hiding this comment.
Although this order is not strict, can we move this new rule after SimplifyConditionals?
There was a problem hiding this comment.
Let's merge your #52495 first then I will rebase this PR and move the rule.
There was a problem hiding this comment.
Fixed in 018055d.
| } | ||
|
|
||
| /** | ||
| * Removes date and time related functions that are unnecessary. |
There was a problem hiding this comment.
Do you think we can elaborate more what is the definition of uncecessary as of now? It would be great an itemized list because we can add more when we improve SimplifyDateTimeConversions in the future.
There was a problem hiding this comment.
Added comments to cases one by one in dcf49a8.
|
@MaxGekk , can you please take a look at these simplifications if you have some time? |
|
Since I merged #52495, could you rebase this PR to the master branch, please? |
peter-toth
force-pushed
the
SPARK-53762-add-simplifydatetimeconversions-rule
branch
from
dcf49a8 to
018055d
Compare
What changes were proposed in this pull request?
This PR add a new rule to the optimizer, that focuses on date and time conversion functions and tries to eliminate the unnecessary ones.
Why are the changes needed?
Date and time conversions are not cheap so eliminating some of them can bring considerable performance improvement.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added new UT.
Was this patch authored or co-authored using generative AI tooling?
No.