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

Allow structs and arrays to pass through for Shuffle and Sort #1477

Merged
merged 5 commits into from
Jan 12, 2021

Conversation

kuhushukla
Copy link
Collaborator

@kuhushukla kuhushukla commented Jan 8, 2021

This change allows structs and arrays for shuffle and sort for pass through as i/p and o/p. The change is fairly simple so I am not sure if more is expected from this change or if I missed anything else. Please guide as needed. I saw some analysis exception with legacy setting for negative scale when trying to sort arrays with decimal type which I am not fully aware on where we stand in terms of support so I excluded those in that particular test.

@kuhushukla kuhushukla added this to the Jan 4 - Jan 15 milestone Jan 8, 2021
@kuhushukla kuhushukla self-assigned this Jan 8, 2021
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.

Is there a reason we don't want to allow for maps too? as they are just an array of structs (assuming that we have tested that it works)

@@ -772,6 +772,7 @@ def gen_scalars_for_sql(data_gen, count, seed=0, force_no_nulls=False):
boolean_gens = [boolean_gen]

single_level_array_gens = [ArrayGen(sub_gen) for sub_gen in all_basic_gens + decimal_gens + [null_gen]]
single_level_array_gens_non_decimal = [ArrayGen(sub_gen) for sub_gen in all_basic_gens + [null_gen]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not include decimal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw some analysis exception with legacy setting for negative scale when trying to sort arrays with decimal type which I am not fully aware on where we stand in terms of support so I excluded those in that particular test.

I will check it out further

@sameerz sameerz added the task Work required that improves the product but is not user facing label Jan 11, 2021
@revans2 revans2 changed the title Whitelist structs and arrays to pass through for Shuffle and Sort Allow structs and arrays to pass through for Shuffle and Sort Jan 12, 2021
Signed-off-by: Kuhu Shukla <[email protected]>
@kuhushukla
Copy link
Collaborator Author

build

revans2
revans2 previously approved these changes Jan 12, 2021
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.

Looks good, but needs an upmerge (sorry conflicts with something I just merged in)

revans2
revans2 previously approved these changes Jan 12, 2021
Signed-off-by: Kuhu Shukla <[email protected]>
@kuhushukla
Copy link
Collaborator Author

build

@kuhushukla kuhushukla merged commit 729bf27 into NVIDIA:branch-0.4 Jan 12, 2021
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
tgravescs pushed a commit to tgravescs/spark-rapids that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Work required that improves the product but is not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants