Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support CTAS #125
Support CTAS #125
Changes from 2 commits
9437101
e80cf51
f976e99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 164 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 164 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 164 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 164 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 517 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 517 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 517 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 517 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 617 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 617 in src/from_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning child here seems weird. what's the reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be useful to document this branch as well with the answer. I'm guessing we're treating this as a no-op for all non-write operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RootOp is always a ProjectionRelation for read operations (existing code). ProjRel is added in line 820. For write_op we should return the child relation without adding projection. Only CTAS needed to handle column names here.
Any unsupported write op we throw an exception above in TransformOp, so here we should just return the child.
I can add below comment in next PR if it is helpful.
// return child relation for other supported write operations (INSERT, DELETE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why would we wouldn't throw here. It sounds like you're saying that code above is going to throw so we just return something random here. And what's wrong with adding a projection relation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this change and open an issue to follow-up on better clarity around why we have these weird specialized cases outside of individual transform ops.
Check warning on line 204 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 204 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 1015 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 1015 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)
Check warning on line 1283 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_amd64, x86_64, x64-osx)
Check warning on line 1283 in src/to_substrait.cpp
GitHub Actions / Build extension binaries / MacOS (osx_arm64, arm64, arm64-osx)