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

Re-enable support for GetJsonObject by default #916

Merged

Conversation

amahussein
Copy link
Collaborator

Signed-off-by: Ahmed Hussein (amahussein) a@ahussein.me

Contributes to #865

Re-enable the support for GetJsonObject as RAPIDS 24.04 is supporting it by default

  • Update the unit-test accordingly
  • Minor change in notes of InMemoryTableScanExec

Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>

Contributes to NVIDIA#865

Re-enable the support for GetJsonObject as RAPIDS 24.04 is supporting it
by default
- Update the unit-test accordingly
- Minor change in notes of `InMemoryTableScanExec`
@amahussein amahussein added bug Something isn't working core_tools Scope the core module (scala) labels Apr 9, 2024
@amahussein amahussein self-assigned this Apr 9, 2024
@@ -18,7 +18,7 @@ AQEShuffleReadExec,TNEW,None,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,S,S,NS,PS,PS,PS
HashAggregateExec,S,None,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,S,PS,NS,PS,PS,PS,NS,NS,NS
ObjectHashAggregateExec,S,None,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,S,PS,NS,PS,PS,PS,NS,NS,NS
SortAggregateExec,S,None,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,S,PS,NS,PS,PS,PS,NS,NS,NS
InMemoryTableScanExec,S,This is disabled by default because there could be complications when using it with AQE. For more details please check https://github.com/NVIDIA/spark-rapids/issues/10603,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,NS,NS,NS,PS,PS,PS,NS,S,S
InMemoryTableScanExec,S,This is disabled by default because there could be complications when using it with AQE with Spark-3.5.0 and Spark-3.5.1. For more details please check https://github.com/NVIDIA/spark-rapids/issues/10603,Input/Output,S,S,S,S,S,S,S,S,PS,S,S,NS,NS,NS,PS,PS,PS,NS,S,S
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR but if it is disabled by default, then second column should have been "NS" which we see for other Execs such as CollectLimitExec, WindowInPandasExec

Copy link
Collaborator Author

@amahussein amahussein Apr 9, 2024

Choose a reason for hiding this comment

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

Thanks @nartal1 !
Yes, I don't like it either. @cindyyuanjiang Can you take a look into fixing this while you are making modifications to the script?
This is happening because the plugin disabled that exec in spark-3.5, and Spark-3.5.1
Since we give Precedence to S over NS, then we end up with S
It does not affect the behavior here, but at least we get rid of it showing up everytime the sync-script is executed until Cindy fixes it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From spark-rapids repo it looks like this is disabled only in "3.5" and supported in all prior versions and since the script to union all the Spark-versions uses the latest one, this is getting the notes from "3.5" but second column is not updated to "NS".

@amahussein amahussein merged commit 10dc1bb into NVIDIA:dev Apr 9, 2024
16 checks passed
@amahussein amahussein deleted the spark-rapids-tools-865-getjsonobj branch April 9, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core_tools Scope the core module (scala)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants