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

Support CTAS #125

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Support CTAS #125

merged 3 commits into from
Nov 19, 2024

Conversation

scgkiran
Copy link
Contributor

No description provided.

src/from_substrait.cpp Outdated Show resolved Hide resolved
return proj->CreateRel(create_table->schema_name, create_table->table_name);
}
default:
return child;
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

@scgkiran scgkiran Nov 18, 2024

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)

Copy link
Contributor

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?

Copy link
Contributor

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.

@jacques-n
Copy link
Contributor

As mentioned, going to merge this or as part of cherry picking from other branch and then reevaluate in subsequent issue.

@jacques-n jacques-n merged commit e84a785 into substrait-io:main Nov 19, 2024
9 checks passed
@scgkiran scgkiran deleted the ctas branch November 19, 2024 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants