alamb commented on code in PR #17580:
URL: https://github.com/apache/datafusion/pull/17580#discussion_r2352309534
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -790,9 +790,26 @@ impl SessionContext {
}
if exist {
- match cmd.if_not_exists {
- true => return self.return_empty_dataframe(),
- false => {
+ match (cmd.if_not_exists, cmd.or_replace) {
Review Comment:
I think it makes sense to make this behavior consistent with
`create_memory_table`
##########
datafusion/sqllogictest/test_files/ddl.slt:
##########
@@ -607,6 +607,19 @@ CREATE TABLE table_without_values(field1 BIGINT, field2
BIGINT);
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
Review Comment:
Can you also please test:
1. Using `CREATE OR REPLACE` when the table doesn't already exist
2. Using `CREATE OR REPLACE` to replace the same name with a different
definition (e.g. a parquet file rather than CSV) and show it is different?
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -790,9 +790,26 @@ impl SessionContext {
}
if exist {
- match cmd.if_not_exists {
- true => return self.return_empty_dataframe(),
- false => {
+ match (cmd.if_not_exists, cmd.or_replace) {
+ (true, false) => return self.return_empty_dataframe(),
+ (false, true) => {
+ let result = self
+ .find_and_deregister(cmd.name.clone(), TableType::Base)
+ .await;
+ match result {
+ Ok(true) => {
+ let table_provider: Arc<dyn TableProvider> =
+ self.create_custom_table(cmd).await?;
+ self.register_table(cmd.name.clone(),
table_provider)?;
+ return self.return_empty_dataframe();
+ }
+ _ => return exec_err!("View '{}' doesn't exist.",
cmd.name),
Review 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?
##########
datafusion/sql/src/parser.rs:
##########
@@ -724,13 +728,29 @@ impl<'a> DFParser<'a> {
/// Parse a SQL `CREATE` statement handling `CREATE EXTERNAL TABLE`
pub fn parse_create(&mut self) -> Result<Statement, DataFusionError> {
- if self.parser.parse_keyword(Keyword::EXTERNAL) {
- self.parse_create_external_table(false)
- } else if self.parser.parse_keyword(Keyword::UNBOUNDED) {
- self.parser.expect_keyword(Keyword::EXTERNAL)?;
- self.parse_create_external_table(true)
+ // TODO: Change sql parser to take in `or_replace: bool` inside
parse_create()
+ if self
Review Comment:
sqlparser does releases every few months, so if we need to implement
something in there it will take a while before we can use it here.
I think this code is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
