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.

Sign up for GitHub

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

Jump to bottom

Fix CustomShuffleReader replacement when decimal types enabled #1685

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Fix CustomShuffleReader replacement when decimal types enabled #1685

merged 5 commits into from
Feb 10, 2021

Conversation

sperlingxx
Copy link
Collaborator

This pull request is to fix the bug of AQE with decimal, which is reported in issue #1670.

@sperlingxx
fix CustomShuffleReader replacement error when decimal enabled
62ac42e 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
fix typo
f88b954 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
Copy link
Collaborator Author

build

@jlowe jlowe added the SQL part of the SQL/Dataframe plugin label Feb 8, 2021
jlowe
jlowe reviewed Feb 8, 2021
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Copyrights need to be updated but otherwise lgtm.

tests/src/test/scala/com/nvidia/spark/rapids/AdaptiveQueryExecSuite.scala
@@ -31,6 +31,7 @@ import org.apache.spark.sql.execution.joins.SortMergeJoinExec
import org.apache.spark.sql.functions.{col, when}
Copy link
Member

Choose a reason for hiding this comment

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

2021 copyrights

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@revans2
Copy link
Collaborator

revans2 commented Feb 8, 2021

@sperlingxx Can this impact other types too? We support shuffling nested types like arrays, maps, and structs, but GpuCustomShuffleReader does not.

@andygrove
Copy link
Contributor

Thanks @sperlingxx . I applied this change to my branch and ran quite a few queries with decimal + AQE enabled and it worked well.

@abellina
Copy link
Collaborator

abellina commented Feb 8, 2021

Is there a way to make this fail in a way that we can fallback to the cpu?

@sperlingxx
update
23d61d4 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
Copy link
Collaborator Author

@sperlingxx Can this impact other types too? We support shuffling nested types like arrays, maps, and structs, but GpuCustomShuffleReader does not.

Hi @revans2, I added ArrayType and StructType as supported types. And MapType is not supported in ShuffleExchangeExec.

@sperlingxx
Copy link
Collaborator Author

Is there a way to make this fail in a way that we can fallback to the cpu?

I believe when we configure spark.rapids.sql.decimalType.enabled with false, it will fallback to cpu. The fallback strategy will fail at here because parent plan(CustomShuffleReaderExec) fallbacks but child plan does not.

@sperlingxx
Copy link
Collaborator Author

build

revans2
revans2 reviewed Feb 9, 2021
View reviewed changes
docs/supported_ops.md Outdated
@@ -9,7 +9,7 @@ support all data types. The RAPIDS Accelerator for Apache Spark has further
restrictions on what types are supported for processing. This tries
to document what operations are supported and what data types each operation supports.
Because Apache Spark is under active development too and this document was generated
against version 3.0.0 of Spark. Most of this should still
against version 3.0.1 of Spark. Most of this should still
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default is still to use 3.0.0 for building the docs. What happened to change it to 3.0.1 when the build itself did not change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad. I ran mvn verify with Dspark.version=3.0.1 to update the doc.

@sperlingxx
fix supported_ops.md
b72778f 
Signed-off-by: sperlingxx <lovedreamf@gmail.com>
@sperlingxx
Copy link
Collaborator Author

build

@sperlingxx sperlingxx merged commit 753e914 into NVIDIA:branch-0.4 Feb 10, 2021
@andygrove andygrove linked an issue Feb 17, 2021 that may be closed by this pull request
[BUG] Some TPC-DS queries fail with AQE when decimal types enabled #1670
Closed
@sperlingxx sperlingxx deleted the fix_decimal_aqe branch February 21, 2021 08:26
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@sperlingxx
Fix CustomShuffleReader replacement when uncommon types enabled (NVID…
8e51e4c 
…IA#1685)

Signed-off-by: sperlingxx <lovedreamf@gmail.com>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@sperlingxx
Fix CustomShuffleReader replacement when uncommon types enabled (NVID…
ebcf1b1 
…IA#1685)

Signed-off-by: sperlingxx <lovedreamf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 left review comments

@jlowe jlowe jlowe approved these changes

@andygrove andygrove Awaiting requested review from andygrove

Assignees
No one assigned
Labels
SQL part of the SQL/Dataframe plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Some TPC-DS queries fail with AQE when decimal types enabled
5 participants
@sperlingxx @revans2 @andygrove @abellina @jlowe