-
Notifications
You must be signed in to change notification settings - Fork 28k
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-15776][SQL] Divide Expression inside Aggregation function is casted to wrong type #13651
Conversation
@@ -2847,4 +2847,15 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext { | |||
test("SPARK-15887: hive-site.xml should be loaded") { | |||
assert(spark.sessionState.newHadoopConf().get("hive.in.test") == "true") | |||
} | |||
|
|||
test("SPARK-15776 Divide expression inside an Aggregation function should not " + |
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.
I think we need some low level unit test instead of end-to-end test
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.
It involves Analyzer, low level unit test is hard to cover that?
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.
TypeCoercionSuite
Test build #60439 has finished for PR 13651 at commit
|
I think we need to explain the inconsistent behavior in the PR description, which is the main reason of making this change. (right now, if we run |
|
||
// Cast integer to double | ||
ruleTest(analyzer, sum(Divide(4, 3)), sum(Divide(Cast(4, DoubleType), Cast(3, DoubleType)))) | ||
// left expression is already Double, skip |
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.
what if left expression is int and right expression is double?
Test build #60515 has finished for PR 13651 at commit
|
3dd03b7
to
ab4ada0
Compare
1787c05
to
c7e0b33
Compare
cc @hvanhovell |
Test build #60527 has finished for PR 13651 at commit
|
Test build #60524 has finished for PR 13651 at commit
|
Test build #60528 has finished for PR 13651 at commit
|
Test build #60529 has finished for PR 13651 at commit
|
Test build #60532 has finished for PR 13651 at commit
|
Test build #60533 has finished for PR 13651 at commit
|
Test build #60535 has finished for PR 13651 at commit
|
Test build #60536 has finished for PR 13651 at commit
|
Test build #60551 has finished for PR 13651 at commit
|
LGTM thank you |
checkConsistencyBetweenInterpretedAndCodegen(Divide, tpe, tpe) | ||
} | ||
} | ||
|
||
test("/ (Divide) for integral type") { | ||
test("SPARK-15776: Divide report unresolved when children's data type is " + |
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 suite is to test the correctness, not resolution.
BTW, the resolution logic is defined by ExpectedInputTypes
, I think we don't need to test it again for divide.
70d4cc6
to
da25d7a
Compare
@hvanhovell @cloud-fan Thanks! Updated. |
LGTM, pending jenkins |
Test build #60586 has finished for PR 13651 at commit
|
Test build #60590 has finished for PR 13651 at commit
|
LGTM, pending jenkins |
Test build #60591 has finished for PR 13651 at commit
|
merging to master/2.0! |
…asted to wrong type ## What changes were proposed in this pull request? This PR fixes the problem that Divide Expression inside Aggregation function is casted to wrong type, which cause `select 1/2` and `select sum(1/2)`returning different result. **Before the change:** ``` scala> sql("select 1/2 as a").show() +---+ | a| +---+ |0.5| +---+ scala> sql("select sum(1/2) as a").show() +---+ | a| +---+ |0 | +---+ scala> sql("select sum(1 / 2) as a").schema res4: org.apache.spark.sql.types.StructType = StructType(StructField(a,LongType,true)) ``` **After the change:** ``` scala> sql("select 1/2 as a").show() +---+ | a| +---+ |0.5| +---+ scala> sql("select sum(1/2) as a").show() +---+ | a| +---+ |0.5| +---+ scala> sql("select sum(1/2) as a").schema res4: org.apache.spark.sql.types.StructType = StructType(StructField(a,DoubleType,true)) ``` ## How was this patch tested? Unit test. This PR is based on #13524 by Sephiroth-Lin Author: Sean Zhong <seanzhong@databricks.com> Closes #13651 from clockfly/SPARK-15776. (cherry picked from commit 9bd80ad) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Test build #60592 has finished for PR 13651 at commit
|
### What changes were proposed in this pull request? The pr aims to upgrade `Netty` from `4.1.100.Final` to `4.1.106.Final`. ### Why are the changes needed? - To bring the latest bug fixes Automatically close Http2StreamChannel when Http2FrameStreamExceptionreaches end ofChannelPipeline ([#13651](netty/netty#13651)) Symbol not found: _netty_jni_util_JNI_OnLoad ([#13695](netty/netty#13728)) - 4.1.106.Final release note: https://netty.io/news/2024/01/19/4-1-106-Final.html - 4.1.105.Final release note: https://netty.io/news/2024/01/16/4-1-105-Final.html - 4.1.104.Final release note: https://netty.io/news/2023/12/15/4-1-104-Final.html - 4.1.103.Final release note: https://netty.io/news/2023/12/13/4-1-103-Final.html - 4.1.101.Final release note: https://netty.io/news/2023/11/09/4-1-101-Final.html ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #44384 from panbingkun/SPARK-46432. Lead-authored-by: panbingkun <panbingkun@baidu.com> Co-authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
What changes were proposed in this pull request?
This PR fixes the problem that Divide Expression inside Aggregation function is casted to wrong type, which cause
select 1/2
andselect sum(1/2)
returning different result.Before the change:
After the change:
How was this patch tested?
Unit test.
This PR is based on #13524 by @Sephiroth-Lin