-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-35170][SQL] Extend BinaryOperator
by SubtractDates
and SubtractTimestamps
#32267
Conversation
@gengliangwang @cloud-fan FYI, this PR changes behavior in errors slightly (I guess it is not critical) :
|
Though, no. It changes the behavior actually - an exception instead of NULL:
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
-- !query output | ||
NULL | ||
org.apache.spark.sql.AnalysisException |
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.
@cloud-fan @gengliangwang Are we ok to change the behavior?
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 doesn't look good...Why does that happen?
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.
Probably because we have special type coercion logic for BinaryOperators?
@@ -1008,7 +1008,7 @@ SELECT cast('2017-12-11 09:30:00.0' as timestamp) - cast(1 as decimal(3, 0)) FRO | |||
struct<> | |||
-- !query output | |||
org.apache.spark.sql.AnalysisException | |||
cannot resolve '(CAST('2017-12-11 09:30:00.0' AS TIMESTAMP) - CAST(1 AS DECIMAL(3,0)))' due to data type mismatch: argument 2 requires timestamp type, however, 'CAST(1 AS DECIMAL(3,0))' is of decimal(3,0) type.; line 1 pos 7 | |||
cannot resolve '(CAST('2017-12-11 09:30:00.0' AS TIMESTAMP) - CAST(1 AS DECIMAL(3,0)))' due to data type mismatch: differing types in '(CAST('2017-12-11 09:30:00.0' AS TIMESTAMP) - CAST(1 AS DECIMAL(3,0)))' (timestamp and decimal(3,0)).; line 1 pos 7 |
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.
From my point of view, old error looks better.
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.
+1
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #137724 has finished for PR 32267 at commit
|
Test build #137733 has finished for PR 32267 at commit
|
I am closing this because:
|
What changes were proposed in this pull request?
In the PR, I propose to modify the
SubtractDates
andSubtractTimestamps
expressions to extendBinaryOperator
instead ofBinaryExpression
.Why are the changes needed?
To improve code maintenance.
Does this PR introduce any user-facing change?
No
How was this patch tested?
By existing test suites.