yihua commented on code in PR #784:
URL: https://github.com/apache/hudi-rs/pull/784#discussion_r4396347052
##########
crates/core/src/table/partition.rs:
##########
@@ -174,6 +174,83 @@ impl PartitionPruner {
})
}
+ /// Returns `true` if a partition directory prefix should be descended
into.
+ ///
+ /// Unlike `should_include`, `partial_path` may resolve fewer segments than
+ /// the schema has fields (we're mid-descent, not at a leaf yet). Any
segment
+ /// this can't safely reason about — not enough schema fields to compare
+ /// against, an opaque single-field `_hoodie_partition_path` schema (e.g.
+ /// under a timestamp-based key generator, where that one field's value
+ /// spans multiple physical path segments, so no individual segment carries
+ /// independently meaningful value), a hive-style segment that doesn't
parse
+ /// as `key=value`, or a cast/compare error — is treated as unconstrained,
+ /// never as a reason to reject. Only a segment that positively fails an
+ /// already-resolved filter returns `false`.
+ ///
+ /// `is_hive_style && is_url_encoded` is required unconditionally, matching
+ /// Java's `FileSystemBackedTableMetadata` (line 157) — regardless of field
+ /// count. `is_hive_style` is needed because without `key=value` labels
+ /// there's no independent way to verify a split segment lines up with its
+ /// schema field at all. `is_url_encoded` is needed even for a single
+ /// field: without it, a value containing a literal `/` splits across an
+ /// extra physical directory level (e.g. `region=b/c` on disk for a value
+ /// `"b/c"`), and a mid-descent prefix that has only resolved the first
+ /// level (`region=b`) has no way to tell that genuinely-short value apart
+ /// from one truncated by such a stray `/` — the `parts.len()` check below
+ /// can't catch it either, since at that depth there simply aren't more
+ /// parts yet to exceed the field count. Comparing the truncated value
+ /// against a filter for the full value would then wrongly reject a prefix
+ /// that should have been descended into.
+ pub fn should_include_prefix(&self, partial_path: &str) -> bool {
+ // A single-field schema is only unsafe to reason about per-segment
when
+ // that field is the opaque `_hoodie_partition_path` metafield, whose
one
+ // value spans multiple physical segments. A genuine single partition
+ // field (e.g. `SimpleKeyGenerator`) has no such gap: its one segment
+ // *is* its full value when `is_hive_style && is_url_encoded` hold, so
+ // it can be compared like any other field.
+ let is_opaque_single_field = self.schema.fields().len() == 1
+ && self.schema.field(0).name() ==
MetaField::PartitionPath.as_ref();
+ if self.and_filters.is_empty() || is_opaque_single_field {
+ return true;
+ }
+ if !(self.is_hive_style && self.is_url_encoded) {
Review Comment:
non-blocking: with the gate as written, a hive-style table without
url-encoding (Spark SQL's default) gets no descent pruning, which is the case
#779 describes, and
`partition_filter_pruning_fails_open_without_url_encoding_single_field` pins
that at 4 calls either way. The slash ambiguity only exists for string-typed
columns, so would it be safe to evaluate a segment whenever every partition
field up to and including it has a non-string type? Fine as a follow-up if
you'd rather keep this PR to the encoded 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]
