mbutrovich commented on code in PR #26943:
URL: https://github.com/apache/datafusion/pull/26943#discussion_r4483020923
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -341,15 +354,28 @@ fn try_into_partitioned_file(
object_meta: ObjectMeta,
partition_cols: &[(String, DataType)],
table_path: &ListingTableUrl,
+ strict: bool,
) -> Result<Option<PartitionedFile>> {
let cols = partition_cols.iter().map(|(name, _)| name.as_str());
let parsed = parse_partitions_for_path(table_path, &object_meta.location,
cols);
let Some(parsed) = parsed else {
+ if strict {
+ return strict_partitions_err(
+ table_path,
+ &object_meta.location,
+ partition_cols,
+ "it is not under a directory for every partition column",
+ );
+ }
// parse_partitions_for_path already logs a debug message
return Ok(None);
};
+ if strict {
+ check_partition_directories(table_path, &object_meta.location,
partition_cols)?;
+ }
Review Comment:
In strict mode, could `check_partition_directories` run before
`parse_partitions_for_path` and check the column names too? Then the `let
Some(parsed) else` branch needs no strict case, and the error can name the
directory that's wrong. Today `wrong_col=2024-01/data.parquet` with
`PARTITIONED BY (year_month)`, and `month=01/year=2024/data.parquet` with
`PARTITIONED BY (year, month)`, both report "it is not under a directory for
every partition column" ([test
cases](https://github.com/apache/datafusion/blob/9cea4cc52256e130ba7e89bdd4f444bda2b08768/datafusion/catalog-listing/src/helpers.rs#L939-L953)),
although both files are under two directories.
A sketch of the loop in `check_partition_directories`, replacing the length
check:
```rust
let mut dirs = dirs.into_iter();
for (col, _) in partition_cols {
let reason = match dirs.next() {
Some(dir) if dir.split_once('=').is_some_and(|(name, _)| name ==
col) => {
continue;
}
Some(dir) => format!("directory '{dir}' does not match partition
column '{col}'"),
None => format!("it is not under a directory for partition column
'{col}'"),
};
return strict_partitions_err(table_path, file_path, partition_cols,
&reason);
}
match dirs.find(|d| d.contains('=')) {
// existing extra-directory error
}
```
Once this check passes, the first `partition_cols.len()` path segments are
directories with the declared names, so `parse_partitions_for_path` returns
`Some`.
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -428,6 +470,60 @@ pub async fn pruned_partition_list<'a>(
}
}
+fn check_partition_directories(
+ table_path: &ListingTableUrl,
+ file_path: &Path,
+ partition_cols: &[(String, DataType)],
+) -> Result<()> {
+ let Some(segments) = table_path.strip_prefix(file_path) else {
+ return strict_partitions_err(
+ table_path,
+ file_path,
+ partition_cols,
+ "it is not under the table path",
+ );
+ };
Review Comment:
Should this be an internal error? `list_prefixed_files` keeps only files
where
[`contains`](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/datasource/src/url.rs#L284-L289)
returns true, and `contains` returns false [when `strip_prefix`
fails](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/datasource/src/url.rs#L161-L164).
So a listed file is always under the table path, and reaching this branch
means an invariant broke. The function already reports one of those with
[`assert_or_internal_err!`](https://github.com/apache/datafusion/blob/9cea4cc52256e130ba7e89bdd4f444bda2b08768/datafusion/catalog-listing/src/helpers.rs#L422-L426).
This would need `internal_err` added to the `datafusion_common` import.
```suggestion
// The listing only yields files under the table path
let Some(segments) = table_path.strip_prefix(file_path) else {
return internal_err!("File '{file_path}' is not under table path
'{table_path}'");
};
```
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -377,6 +403,10 @@ pub async fn pruned_partition_list<'a>(
file_extension: &'a str,
partition_cols: &'a [(String, DataType)],
) -> Result<BoxStream<'a, Result<PartitionedFile>>> {
+ let strict = ctx
+ .config_options()
+ .execution
+ .listing_table_strict_partitions;
Review Comment:
Should `INSERT INTO` fail in strict mode? `insert_into_inner` also calls
[`pruned_partition_list`](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/catalog-listing/src/table.rs#L841-L851),
so with a stray root file, `insert into t values (5, 'eve', '2026')` now fails
with the same planning error. As I read it, that listing only feeds the sink's
`file_group`, which is used for [`EXPLAIN`
output](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/datasource-parquet/src/sink.rs#L90)
and [protobuf
serialization](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/datasource/src/file_sink_config/proto.rs#L24-L28),
so the write doesn't depend on the existing layout. If writes should be
checked, could the config doc say so and the `.slt` cover it? If not, the
insert path needs a way to skip the check. `pruned_partition_list` is public,
so changing its signatur
e to pass `strict` in is an API break.
##########
datafusion/sqllogictest/test_files/listing_table_partitions.slt:
##########
@@ -127,3 +140,120 @@ select year, count(*) from root_file_test group by year
order by year;
----
2024 2
2025 1
+
+# Test: with listing_table_strict_partitions, files outside the partition
+# structure are an error instead of being skipped
+
+# Disable the list files cache so the stale root file, written after the
+# table was created, is listed
+statement count 0
+set datafusion.runtime.list_files_cache_limit = "0K";
+
+query I
+select count(*) from root_file_test;
+----
+3
+
+statement count 0
+set datafusion.execution.listing_table_strict_partitions = true;
+
+query error DataFusion error: Error during planning: File
'.*root_file_skipped/stale\.parquet' in table '.*' does not match partition
columns \["year"\]: it is not under a directory for every partition column\.
Set datafusion\.execution\.listing_table_strict_partitions = false to skip such
files
+select count(*) from root_file_test;
+
+# Only listed files are checked: a partition filter narrows the listing
+# prefix to year=2025/, so the root file is never listed
+query ITT
+select column1, column2, year from root_file_test where year = '2025';
+----
+3 charlie 2025
+
+# Declaring fewer partition columns than the directories have is an error
+statement count 0
+create external table multi_part_only_a
+stored as parquet
+partitioned by (a)
+location 'test_files/scratch/listing_table_partitions/multi_part/';
+
+query error DataFusion error: Error during planning: File
'.*multi_part/a=1/b=[0-9]+/file1\.parquet' in table '.*' does not match
partition columns \["a"\]: directory 'b=[0-9]+' is not a declared partition
column
+select * from multi_part_only_a;
+
+# Also for tables without partition columns
+query error DataFusion error: Error during planning: File
'.*multi_part/a=1/b=[0-9]+/file1\.parquet' in table '.*' does not match
partition columns \[\]: directory 'a=1' is not a declared partition column
+select * from infer_disabled;
+
+# Matching files are read as usual
+query TTT
+select * from multi_part order by column1;
+----
+bar 1 100
+baz 1 200
+foo 1 100
+
+query I
+copy (values(1))
+to
'test_files/scratch/listing_table_partitions/partition_filename/year=2024/a=b.parquet';
+----
+1
+
+statement count 0
+create external table partition_filename (column1 bigint, year varchar)
+stored as parquet
+partitioned by (year)
+location 'test_files/scratch/listing_table_partitions/partition_filename/';
+
+query IT
+select * from partition_filename;
+----
+1 2024
+
+query I
+copy (values(2))
+to
'test_files/scratch/listing_table_partitions/partition_filename/year=2025.parquet';
+----
+1
+
+query error File '.*partition_filename/year=2025\.parquet'.*does not match
partition columns \["year"\]: it is not under a directory for every partition
column
+select * from partition_filename;
+
+query I
+copy (values(3))
+to
'test_files/scratch/listing_table_partitions/nested_partition_filename/year=2024/month=01/a=b.parquet';
+----
+1
+
+statement count 0
+create external table nested_partition_filename (column1 bigint, year varchar,
month varchar)
+stored as parquet
+partitioned by (year, month)
+location
'test_files/scratch/listing_table_partitions/nested_partition_filename/';
+
+query ITT
+select * from nested_partition_filename;
+----
+3 2024 01
+
+query I
+copy (values(4))
+to
'test_files/scratch/listing_table_partitions/nested_partition_filename/year=2024/month=02.parquet';
+----
+1
+
+query error File
'.*nested_partition_filename/year=2024/month=02\.parquet'.*does not match
partition columns \["year", "month"\]: it is not under a directory for every
partition column
+select * from nested_partition_filename;
+
+statement count 0
+set datafusion.execution.listing_table_strict_partitions = false;
+
+statement count 0
+reset datafusion.runtime.list_files_cache_limit;
+
+statement count 0
+reset datafusion.catalog.create_default_catalog_and_schema;
+
Review Comment:
Why reset `datafusion.catalog.create_default_catalog_and_schema` here?
Nothing in this file sets it, so the statement looks like a leftover. Could we
drop it?
```suggestion
```
--
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]
