-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
HIVE-28302: Let SUM UDF return NULL when all rows have non-numeric texts #5283
base: master
Are you sure you want to change the base?
Conversation
@@ -92,4 +92,4 @@ FROM src | |||
POSTHOOK: type: QUERY | |||
POSTHOOK: Input: default@src | |||
#### A masked pattern was here #### | |||
0.0 NULL NULL NULL | |||
NULL NULL NULL NULL |
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.
We may remove this test case as cbo_aggregate_reduce_functions_rule.q
covers the case.
https://github.com/apache/hive/blob/master/ql/src/test/queries/clientpositive/udaf_number_format.q
In the test cbo_aggregate_reduce_functions_rule.q aggregate functions under test (sum, count, stddev*, etc) are wrapped into a
returns invalid result it can have multiple reasons: WDYT? |
|
@kasakrisz I don't have any objections. The original intention is human-readability and floating-point error mitigation. I tried to remove ROUND. To be fair, we observe difference between CBO and non-CBO.
For better visibility,
I presume they are acceptable as floating-point errors. |
What changes were proposed in this pull request?
Make SUM return not 0.0 but NULL when both of the following conditions are satisfied.
https://issues.apache.org/jira/browse/HIVE-28302
Why are the changes needed?
I understand the SQL standard requires a UDAF to return NULL when all rows are NULL. Actually, other UDAFs such as AVG behave like that.
We can see some more discussions in #5091
Does this PR introduce any user-facing change?
Yes. But I believe the current behavior is not an intentional one but a kind of bug.
Is the change a dependency upgrade?
No.
How was this patch tested?
Updated integration tests.