You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/15 21:27:43 UTC
[GitHub] [spark] cashmand opened a new pull request #35863: Remove unnecessary casts between IntegerType and IntDecimal
cashmand opened a new pull request #35863:
URL: https://github.com/apache/spark/pull/35863
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
In `NTile`, the number of rows per bucket is computed as `n / buckets`, where `n` is the partition size, and `buckets` is the argument to `NTile` (number of buckets). The code currently casts the arguments to IntDecimal, then casts the result back to IntegerType. This is unnecessary, since it is equivalent to just doing integer division, i.e. `n div buckets`. This PR makes that simplifying change.
### Why are the changes needed?
Simplifies the code, and avoids a couple of casts at run-time.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Relying on existing tests (specifically, org.apache.spark.sql.hive.execution.WindowQuerySuite).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35863: [SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35863:
URL: https://github.com/apache/spark/pull/35863#discussion_r827658827
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala
##########
@@ -825,7 +825,7 @@ case class NTile(buckets: Expression) extends RowNumberLike with SizeBasedWindow
zero,
zero,
zero,
- (n.cast(DecimalType.IntDecimal) / buckets.cast(DecimalType.IntDecimal)).cast(IntegerType),
+ (n div buckets).cast(IntegerType),
Review comment:
yea, it's a new fix to replace https://github.com/apache/spark/commit/ec8a1a8e8869b000a8001126ca013b753acddf6a
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala
##########
@@ -825,7 +825,7 @@ case class NTile(buckets: Expression) extends RowNumberLike with SizeBasedWindow
zero,
zero,
zero,
- (n.cast(DecimalType.IntDecimal) / buckets.cast(DecimalType.IntDecimal)).cast(IntegerType),
+ (n div buckets).cast(IntegerType),
Review comment:
yea, it's a better fix to replace https://github.com/apache/spark/commit/ec8a1a8e8869b000a8001126ca013b753acddf6a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #35863: [SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35863:
URL: https://github.com/apache/spark/pull/35863#issuecomment-1068774359
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35863: [SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35863:
URL: https://github.com/apache/spark/pull/35863#discussion_r827587558
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala
##########
@@ -825,7 +825,7 @@ case class NTile(buckets: Expression) extends RowNumberLike with SizeBasedWindow
zero,
zero,
zero,
- (n.cast(DecimalType.IntDecimal) / buckets.cast(DecimalType.IntDecimal)).cast(IntegerType),
+ (n div buckets).cast(IntegerType),
Review comment:
Looks like it's a partial revert of https://github.com/apache/spark/commit/ec8a1a8e8869b000a8001126ca013b753acddf6a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cashmand commented on pull request #35863: [SPARK-38558] Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
cashmand commented on pull request #35863:
URL: https://github.com/apache/spark/pull/35863#issuecomment-1068645295
Thanks @weixiuli, I created https://issues.apache.org/jira/browse/SPARK-38558. I couldn't figure out how to assign it to myself. I'm not sure if I don't have permission, or if I just missed something in the UI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on pull request #35863: Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
weixiuli commented on pull request #35863:
URL: https://github.com/apache/spark/pull/35863#issuecomment-1068605494
@cashmand should we create a jira ticket and use the ticket id as pr title? may like `[SPARK-xxx][SQL] Remove unnecessary casts between IntegerType and IntDecimal.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #35863: [SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35863:
URL: https://github.com/apache/spark/pull/35863
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org