feat: Add OR REPLACE to creating external tables
#17580
Conversation
github-actions
bot
added
sql
There was a problem hiding this comment.
Thank you @jonathanc-n -- I think this is very close.
I think it needs a few more tests and a small fix to the creation logic but then it will be good to go.
Thanks again
| match cmd.if_not_exists { | ||
| true => return self.return_empty_dataframe(), | ||
| false => { | ||
| match (cmd.if_not_exists, cmd.or_replace) { |
There was a problem hiding this comment.
I think it makes sense to make this behavior consistent with create_memory_table
| self.register_table(cmd.name.clone(), table_provider)?; | ||
| return self.return_empty_dataframe(); | ||
| } | ||
| _ => return exec_err!("View '{}' doesn't exist.", cmd.name), |
There was a problem hiding this comment.
I think the idea is that CREATE OR REPLACE EXTERNA TABLE ... will succeed regardless of if the table already exists (that is what create_memory_table) does. Can you please make this similarly consistent?
There was a problem hiding this comment.
This entire clause is wrapped by an exist flag, so it shouldnt be able to return Ok(false). I will refactor the whole logic to be similar to how memory table handles the flags to make it more clear
There was a problem hiding this comment.
I added the full code functionality for each branch to make it more clear
| statement error Execution error: 'IF NOT EXISTS' cannot coexist with 'REPLACE' | ||
| CREATE OR REPLACE TABLE IF NOT EXISTS table_without_values(field1 BIGINT, field2 BIGINT); | ||
|
|
||
| # CREATE OR REPLACE |
There was a problem hiding this comment.
Can you also please test:
- Using
CREATE OR REPLACEwhen the table doesn't already exist - Using
CREATE OR REPLACEto replace the same name with a different definition (e.g. a parquet file rather than CSV) and show it is different?
|
Added the extra tests + clarified the logic! Should be good for another look |
|
Merge wasn't working at the time of review but it is! Going to merge if there are no objections |
|
Let's do it @blaginin ! |
|
gogogogo -- the code is flowing now. Love it |
|
maybe we should do it more often haha 😄 |
3 participants
Which issue does this PR close?
OR REPLACEforCREATE EXTERNAL TABLE#17496 .What changes are included in this PR?
Added parsing for OR REPLACE in create external tables + functionality for execution
Are these changes tested?
Yes slt tests and unit tests for the parser