Skip to content
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

Implement SumUnboundedToUnboundedFixer #8934

Merged
merged 21 commits into from
Aug 16, 2023

Conversation

andygrove
Copy link
Contributor

@andygrove andygrove commented Aug 4, 2023

Closes #6560

Status

  • Implement new SumUnboundedToUnboundedFixer
    • Integral types
    • Floating-point types
    • Decimals
  • Integration test
  • Configs:
    • Something that would let us disable this optimization for floats/doubles because the order of operations would change and could produce different results from one run to the next.

The following requirements from the issue are not implemented yet, and I have filed #8943 for these since they are not specific to sum operations.

  • A config that would let us disable window operations that require a single batch for all data (any window operations with no partition by and is not batch-able)
  • A config that would let us disable window operations that require a batch for an entire partition by group. (any window operation with a partition by but is not batch-able)

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
@andygrove andygrove self-assigned this Aug 4, 2023
--signoff

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs
add comment

Signed-off-by: Andy Grove <andygrove@nvidia.com>

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs

Verified

This commit was signed with the committer’s verified signature.
BethGriggs Bethany Griggs
@abellina
Copy link
Collaborator

abellina commented Aug 4, 2023

For my own edification, why do we need the configs for the single-batch windows?

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
@andygrove
Copy link
Contributor Author

For my own edification, why do we need the configs for the single-batch windows?

I copied the requirements from the issue. I assume we want a way to work around the case where the single-batch does not fit into GPU memory.

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
@andygrove
Copy link
Contributor Author

build

@andygrove andygrove changed the title WIP: Implement SumUnboundedToUnboundedFixer Implement SumUnboundedToUnboundedFixer Aug 7, 2023
@andygrove andygrove marked this pull request as ready for review August 7, 2023 22:15
revans2
revans2 previously approved these changes Aug 8, 2023
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

The code and tests look good. I just think that we might have some dead code in there and it would be nice to clean it up if that is true.

integration_tests/src/main/python/window_function_test.py Outdated Show resolved Hide resolved
previousValue = Some(Scalar.fromDouble(scalar.getDouble + prev.getDouble))
case DType.DTypeEnum.DECIMAL32 | DType.DTypeEnum.DECIMAL64 |
DType.DTypeEnum.DECIMAL128 =>
val sum = prev.getBigDecimal.add(scalar.getBigDecimal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need overflow checking here too?

@sameerz sameerz added the reliability Features to improve reliability or bugs that severly impact the reliability of the plugin label Aug 9, 2023
@sameerz
Copy link
Collaborator

sameerz commented Aug 9, 2023

Do we expect any performance implications due to this change?

@revans2
Copy link
Collaborator

revans2 commented Aug 9, 2023

Do we expect any performance implications due to this change?

I would expect very little performance change. The main goal is that we don't have to hold all of the data in memory at once so we can spill if needed.

revans2
revans2 previously approved these changes Aug 11, 2023
previousValue = Some(Scalar.fromDouble(scalar.getDouble + prev.getDouble))
case DType.DTypeEnum.DECIMAL32 | DType.DTypeEnum.DECIMAL64 |
DType.DTypeEnum.DECIMAL128 =>
withResource(ColumnVector.fromScalar(scalar, 1)) { scalarCv =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Could we just do what spark does on the CPU? We already know what the decimal type should be.

The following was copied from Add, and can be slightly modified to work I think.

private lazy val numeric = TypeUtils.getNumeric(dataType, failOnError)

checkDecimalOverflow(numeric.plus(input1, input2).asInstanceOf[Decimal], precision, scale)

  protected def checkDecimalOverflow(value: Decimal, precision: Int, scale: Int): Decimal = {
    value.toPrecision(precision, scale, Decimal.ROUND_HALF_UP, !failOnError, getContextOrNull())
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have updated this.

@revans2
Copy link
Collaborator

revans2 commented Aug 14, 2023

build

@andygrove andygrove merged commit 8927411 into NVIDIA:branch-23.10 Aug 16, 2023
@andygrove andygrove deleted the sum-unbounded branch August 16, 2023 14:58
andygrove added a commit to andygrove/spark-rapids that referenced this pull request Aug 17, 2023
andygrove added a commit to andygrove/spark-rapids that referenced this pull request Aug 17, 2023
This reverts commit 8927411.

Signed-off-by: Andy Grove <andygrove@nvidia.com>
abellina pushed a commit that referenced this pull request Aug 18, 2023
This reverts commit 8927411.

Signed-off-by: Andy Grove <andygrove@nvidia.com>
andygrove added a commit to andygrove/spark-rapids that referenced this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reliability Features to improve reliability or bugs that severly impact the reliability of the plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Optimize memory for SUM on unbounded preceding to unbounded following.
4 participants