Muneer320 commented on code in PR #27151:
URL: https://github.com/apache/datafusion/pull/27151#discussion_r4538935715
##########
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:
Yes. The rustdoc now says that the final component is always the filename,
including when it contains `=`. It documents the required ordered partition
directories and the `None` result for missing or mismatched directories, with a
complete versus incomplete path example.
##########
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:
Agreed: this changes observable behavior, not just which strings the helper
accepts. The description now calls out the integer-partition example
explicitly: a root `year=2025.parquet` file arriving after table registration
changes from a cast error to a skipped file, so the query can succeed with
fewer rows. I removed the blanket “no public API change” wording and linked
#26943 as the proposed opt-in strict counterpart, without implying that option
is already available.
The SQL reproducer registers the valid layout first: creating the table over
this two-file mixed layout is rejected earlier by partition inference.
##########
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:
Added both assertions to `test_parse_partitions_for_path`, plus a control
showing that a `key=value` filename remains valid under a complete partition
directory. There is also a SQL regression for the integer-partition root-file
example so the query-level effect is covered.
--
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]
