Skip to content

[SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal#35863

Closed
cashmand wants to merge 1 commit intoapache:masterfrom
cashmand:remove_decimal_cast
Closed

[SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal#35863
cashmand wants to merge 1 commit intoapache:masterfrom
cashmand:remove_decimal_cast

Conversation

@cashmand
Copy link
Contributor

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).

@github-actions github-actions bot added the SQL label Mar 15, 2022
@weixiuli
Copy link
Contributor

@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.

@cashmand cashmand changed the title Remove unnecessary casts between IntegerType and IntDecimal [SPARK-38558] Remove unnecessary casts between IntegerType and IntDecimal Mar 16, 2022
@cashmand
Copy link
Contributor Author

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.

@cashmand cashmand changed the title [SPARK-38558] Remove unnecessary casts between IntegerType and IntDecimal [SPARK-38558][SQL] Remove unnecessary casts between IntegerType and IntDecimal Mar 16, 2022
zero,
zero,
(n.cast(DecimalType.IntDecimal) / buckets.cast(DecimalType.IntDecimal)).cast(IntegerType),
(n div buckets).cast(IntegerType),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's a partial revert of ec8a1a8

Copy link
Contributor

@cloud-fan cloud-fan Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it's a better fix to replace ec8a1a8

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 1acadf3 Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants