Skip to content

Conversation

@charlesbluca
Copy link
Collaborator

Adds support for CREATE TABLE AS and CREATE VIEW AS statements. Some follow up stuff to look into:

  • parsing the schema_name through DataFusion instead of hard-coding
  • parsing whether or not to persist created tables instead of hard-coding
  • consolidating the shared logic between CreateTableAsPlugin.convert and Context.sql in one utility function

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

❗ No coverage uploaded for pull request base (datafusion-sql-planner@f948429). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             datafusion-sql-planner     #656   +/-   ##
=========================================================
  Coverage                          ?   66.13%           
=========================================================
  Files                             ?       73           
  Lines                             ?     3635           
  Branches                          ?      754           
=========================================================
  Hits                              ?     2404           
  Misses                            ?     1081           
  Partials                          ?      150           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Minor suggestions but generally things lgtm!

Some(create_memory_table) => Ok(format!("{}", create_memory_table.name)),
None => match &self.create_view {
Some(create_view) => Ok(format!("{}", create_view.name)),
None => panic!("Encountered a non CreateMemoryTable/CreateView type in get_name"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try replacing the panic with a python error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point - might also be worth it to do a follow up PR applying this change to some of the other plan types, which also panic in similar situations

Copy link
Collaborator

@jdye64 jdye64 left a comment

Choose a reason for hiding this comment

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

Few nits but overall looks good. Thanks!

Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Thanks for this! Changes lgtm!

Copy link
Collaborator

@jdye64 jdye64 left a comment

Choose a reason for hiding this comment

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

LGTM

@jdye64 jdye64 merged commit c830cb5 into dask-contrib:datafusion-sql-planner Aug 8, 2022
@charlesbluca charlesbluca deleted the df-create-table-as branch August 8, 2022 19:23
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.

4 participants