xanderbailey commented on code in PR #27151:
URL: https://github.com/apache/datafusion/pull/27151#discussion_r4528834491
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -475,10 +475,18 @@ pub fn parse_partitions_for_path<'a, I>(
where
I: IntoIterator<Item = &'a str>,
{
- let subpath = table_path.strip_prefix(file_path)?;
+ let mut subpath = table_path.strip_prefix(file_path)?.peekable();
Review Comment:
`parse_partitions_for_path` is public and this changes its contract, do you
think we could document that?
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -475,10 +475,18 @@ pub fn parse_partitions_for_path<'a, I>(
where
I: IntoIterator<Item = &'a str>,
{
- let subpath = table_path.strip_prefix(file_path)?;
+ let mut subpath = table_path.strip_prefix(file_path)?.peekable();
let mut part_values = vec![];
- for (part, expected_partition) in subpath.zip(table_partition_cols) {
+ for expected_partition in table_partition_cols {
+ let part = subpath.next()?;
+ // A partition value must come from a directory, never the filename.
+ if subpath.peek().is_none() {
Review Comment:
One user-facing effect worth adding to the description: with a non-string
partition type, e.g. `PARTITIONED BY (year INT)` with a root file
`year=2025.parquet`, this used to fail the query trying to cast `2025.parquet`,
and now that file is silently skipped. I think skipping is right and consistent
with other incomplete paths, but it does turn a loud failure into missing rows.
#26943 adds `datafusion.execution.listing_table_strict_partitions` for anyone
who wants that to be an error instead so if we can land these together then I
think this is great.
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -875,28 +883,27 @@ mod tests {
("year".to_string(), DataType::Utf8),
("month".to_string(), DataType::Utf8),
];
- let meta = ObjectMeta {
- location: Path::from("bucket/mytable/year=2024/data.parquet"),
- last_modified: chrono::Utc::now(),
- size: 100,
- e_tag: None,
- version: None,
- };
+ for filename in ["data.parquet", "month=02.parquet"] {
Review Comment:
Could we also add a couple of assertions to
`test_parse_partitions_for_path`, since that's the function that changed?
Probably the most common real-world case is a single-level stray file at the
table root:
```rust
assert_eq!(
None,
parse_partitions_for_path(
&ListingTableUrl::parse("file:///bucket/mytable").unwrap(),
&Path::from("bucket/mytable/mypartition=v1.csv"),
vec!["mypartition"]
)
);
```
plus the two-level `mypartition=v1/otherpartition=v2.csv` case.
--
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]
