huan233usc commented on code in PR #2575:
URL: https://github.com/apache/iceberg-rust/pull/2575#discussion_r3346357457
##########
crates/integrations/datafusion/src/table/table_provider_factory.rs:
##########
@@ -181,6 +201,64 @@ fn check_cmd(cmd: &CreateExternalTable) -> Result<()> {
Ok(())
}
+/// Validates the `PARTITIONED BY` columns from a `CREATE EXTERNAL TABLE`
command
+/// against the table's default partition spec.
+///
+/// `CREATE EXTERNAL TABLE ... STORED AS ICEBERG` loads an existing table, so
the
+/// partitioning is fully determined by the Iceberg metadata. DataFusion's
grammar only
+/// accepts plain column names in `PARTITIONED BY` (it cannot express
transforms such as
+/// `bucket[N]` or `day`), so the clause is only supported for
identity-partitioned tables.
+/// For an identity transform the partition field name equals its source
column name, so the
+/// clause must list those column names in order.
+///
+/// An empty clause (no `PARTITIONED BY`) skips this check: any table,
including one with
+/// non-identity transforms, can still be registered for read-only access
without declaring
+/// its partitioning.
+fn validate_partition_columns(table: &Table, declared_partition_cols:
&[String]) -> Result<()> {
+ if declared_partition_cols.is_empty() {
Review Comment:
The behavior here is open for discussion.
We could choose ignore validation partition spec, pros is it will unblock
user creating an external table that is partitioned(potentially with the case
data fusion not supported), cons is the sql is not strictly accurate.
--
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]
