-
Notifications
You must be signed in to change notification settings - Fork 28.8k
[SPARK-52617][SQL] Cast TIME to/from TIMESTAMP_NTZ #51381
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: master
Are you sure you want to change the base?
Changes from all commits
8799de5
c37bbf0
352aacb
7faed87
11c3a26
4be2758
e020fae
5b9a10e
d6f4424
f60c6cb
828d214
90482d6
a9d1bdd
fdc9b92
b919e4e
e2ef8af
0bada28
2a5e9ad
7d77c1f
cb6ad55
cd503a5
8bc58da
8a30b2a
fd1aef3
5c6c97f
6169ea4
29ca860
011b254
ca02825
72bb9b5
68f0c98
6b5ed62
e53bfe3
b488aae
867967e
ebf16ed
58c0bea
b82e21c
4239bef
fd5e76e
89dfef0
68dc4a9
76abdaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,12 @@ package org.apache.spark.sql.catalyst.analysis.resolver | |
|
||
import org.apache.spark.sql.catalyst.expressions.{ | ||
Cast, | ||
CurrentDate, | ||
DefaultStringProducingExpression, | ||
Expression, | ||
TimeZoneAwareExpression | ||
} | ||
import org.apache.spark.sql.types.StringType | ||
MakeTimestampNTZ, | ||
TimeZoneAwareExpression} | ||
import org.apache.spark.sql.types.{StringType, TimestampNTZType, TimeType} | ||
|
||
/** | ||
* Resolves [[TimeZoneAwareExpressions]] by applying the session's local timezone. | ||
|
@@ -58,12 +59,14 @@ class TimezoneAwareExpressionResolver(expressionResolver: ExpressionResolver) | |
expressionWithResolvedChildren, | ||
traversals.current.sessionLocalTimeZone | ||
) | ||
|
||
coerceExpressionTypes( | ||
expression = expressionWithResolvedChildrenAndTimeZone, | ||
expressionTreeTraversal = traversals.current | ||
) match { | ||
expressionTreeTraversal = traversals.current) match { | ||
case cast: Cast if traversals.current.defaultCollation.isDefined => | ||
tryCollapseCast(cast, traversals.current.defaultCollation.get) | ||
case Cast(child, TimestampNTZType, _, _) if child.dataType.isInstanceOf[TimeType] => | ||
MakeTimestampNTZ(CurrentDate(), child) | ||
SubhamSinghal marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+68
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to have this if we have a type coercion rule. Type coercion rules run implicitly in single-pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mihailotim-db are you suggesting to revert all change in TimeZoneAwareExpressionResolver class and type coercion rule will take care of single pass and fixed point analyzer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My take on this is that we should have a separate Analyzer rule (not type coercion rule) that does this transformation given that this cast is user specified. Because it might affect schema I would put this rule after the main resolution batch to avoid any alias computation issues. In that case, we would need this code in single-pass analyzer as well. |
||
case other => | ||
other | ||
} | ||
|
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.
Why is this a separate rule? I think we should be able to make it part of one of the existing rule, maybe
DateTimeOperations
@MaxGekk ?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.
Actually is this Cast coming from internal type coercion or user specified casts? If former, we should perform this change in the place where we add a cast. If latter, we should have an analysis rule for rewrite and not a type coercion one, since there is no coercion going on 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.
@MaxGekk Can you help here?