ethan-tyler commented on code in PR #19633:
URL: https://github.com/apache/datafusion/pull/19633#discussion_r2660374811
##########
datafusion/sql/src/statement.rs:
##########
@@ -1362,6 +1362,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
exec_err!("Function name not provided")
}
}
+ Statement::Truncate { table_names, .. } => {
+ if table_names.len() != 1 {
+ return not_impl_err!(
+ "TRUNCATE with multiple tables is not supported"
+ );
+ }
Review Comment:
If you add the option rejection, some negative tests would round it out:
```
statement error TRUNCATE with CASCADE/RESTRICT is not supported
TRUNCATE TABLE t1 CASCADE;
statement error TRUNCATE with multiple tables is not supported
TRUNCATE TABLE t1, t2;
```
##########
datafusion/sql/src/statement.rs:
##########
@@ -1362,6 +1362,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
exec_err!("Function name not provided")
}
}
+ Statement::Truncate { table_names, .. } => {
+ if table_names.len() != 1 {
+ return not_impl_err!(
+ "TRUNCATE with multiple tables is not supported"
+ );
+ }
+
+ let target = &table_names[0]; // TruncateTableTarget
+ let table =
self.object_name_to_table_reference(target.name.clone())?;
+ let source =
self.context_provider.get_table_source(table.clone())?;
+
+ Ok(LogicalPlan::Dml(DmlStatement {
+ table_name: table.clone(),
+ target: source,
+ op: WriteOp::Truncate,
+ input: Arc::new(LogicalPlan::EmptyRelation(EmptyRelation {
+ produce_one_row: false,
+ schema: DFSchemaRef::new(DFSchema::empty()),
+ })),
+ output_schema: DFSchemaRef::new(DFSchema::empty()),
+ }))
+ }
Review Comment:
This should use DmlStatement::new() rather than constructing the struct
directly. The constructor sets output_schema: make_count_schema() which gives
you the {count: UInt64} that DML ops return.
Also noticed this causes a proto roundtrip mismatch: encode uses the struct
directly but decode uses the constructor (at mod.rs:978), so you'd get
different plans before/after serialization.
Something like:
```suggestion
Ok(LogicalPlan::Dml(DmlStatement::new(
table.clone(),
source,
WriteOp::Truncate,
Arc::new(LogicalPlan::EmptyRelation(EmptyRelation {
produce_one_row: false,
schema: DFSchemaRef::new(DFSchema::empty()),
})),
)))
```
##########
datafusion/sql/src/statement.rs:
##########
@@ -1362,6 +1362,28 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
exec_err!("Function name not provided")
}
}
+ Statement::Truncate { table_names, .. } => {
+ if table_names.len() != 1 {
+ return not_impl_err!(
+ "TRUNCATE with multiple tables is not supported"
+ );
+ }
Review Comment:
The .. here silently drops partitions, identity, cascade, on_cluster, and
target.only. If someone writes TRUNCATE TABLE t CASCADE expecting cascade
behavior, they'd get a normal truncate with no indication the option was
ignored.
Worth rejecting these explicitly? Something like:
```suggestion
Statement::Truncate { table_names, partitions, identity,
cascade, on_cluster, .. } => {
if table_names.len() != 1 {
return not_impl_err!("TRUNCATE with multiple tables is not
supported");
}
let target = &table_names[0];
if target.only {
return not_impl_err!("TRUNCATE with ONLY is not supported");
}
if partitions.is_some() {
return not_impl_err!("TRUNCATE with PARTITION is not supported");
}
if identity.is_some() {
return not_impl_err!("TRUNCATE with RESTART/CONTINUE IDENTITY is not
supported");
}
if cascade.is_some() {
return not_impl_err!("TRUNCATE with CASCADE/RESTRICT is not
supported");
}
if on_cluster.is_some() {
return not_impl_err!("TRUNCATE with ON CLUSTER is not supported");
}
// ... rest
}
```
##########
datafusion/catalog/src/table.rs:
##########
@@ -353,6 +353,14 @@ pub trait TableProvider: Debug + Sync + Send {
) -> Result<Arc<dyn ExecutionPlan>> {
not_impl_err!("UPDATE not supported for {} table", self.table_type())
}
+
+ /// Remove all rows from the table.
+ ///
+ /// Returns an [`ExecutionPlan`] producing a single row with `count`
(UInt64),
+ /// representing the number of rows removed.
+ async fn truncate(&self, _state: &dyn Session) -> Result<Arc<dyn
ExecutionPlan>> {
+ not_impl_err!("TRUNCATE not supported for {}", self.table_type())
Review Comment:
nit: DELETE/UPDATE messages say "... for {} table" but this one drops "table"
--
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]
