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

Add TypeSig checks for join keys and other special cases #3183

Merged
merged 9 commits into from
Aug 18, 2021

Conversation

andygrove
Copy link
Contributor

@andygrove andygrove commented Aug 10, 2021

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

Closes #2102

This PR allows us to provide TypeSig signatures for join keys and generates appropriate documentation in support_ops.md

This is implemented for joins and WindowExec.

Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove andygrove added the documentation Improvements or additions to documentation label Aug 10, 2021
@andygrove andygrove added this to the Aug 2 - Aug 13 milestone Aug 10, 2021
@andygrove andygrove self-assigned this Aug 10, 2021
@andygrove andygrove requested review from revans2 and tgravescs August 10, 2021 16:43
Comment on lines -235 to -237
.withPsNote(TypeEnum.ARRAY, "Cannot be used as join key")
.withPsNote(TypeEnum.STRUCT, "Cannot be used as join key")
.withPsNote(TypeEnum.MAP, "Cannot be used as join key")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just so I am not misunderstanding, are these unrelated changes and more like and audit that you ran into while making the doc change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to this PR we relied on the "ps notes" to document the fact that we don't support map/struct/array as join keys, but we now have specific lines in the supported_ops table for leftKeys and rightKeys for the join operators, showing which types are supported, so we no longer need these ps notes.

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.

This looks really good and is a lot cleaner than what I had in mind.

Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
revans2
revans2 previously approved these changes Aug 11, 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.

I just had one more thing to add to my wish list for this. I think it looks great. The next step would be too apply it to other areas that also need this type of check.

Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove andygrove changed the title WIP: Add TypeSig checks for join keys Add TypeSig checks for join keys and other special cases Aug 11, 2021
@andygrove andygrove marked this pull request as ready for review August 11, 2021 18:37
jlowe
jlowe previously approved these changes Aug 13, 2021
@jlowe
Copy link
Member

jlowe commented Aug 13, 2021

build

@andygrove
Copy link
Contributor Author

build

revans2
revans2 previously approved these changes Aug 17, 2021
@pxLi
Copy link
Collaborator

pxLi commented Aug 18, 2021

build

@revans2 revans2 merged commit 3d73d61 into NVIDIA:branch-21.10 Aug 18, 2021
razajafri pushed a commit to razajafri/spark-rapids that referenced this pull request Aug 23, 2021
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@andygrove andygrove deleted the join-key-supported-types branch November 30, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Supported ops page differentiate type supported in key vs passed along
6 participants