yihua commented on code in PR #724:
URL: https://github.com/apache/hudi-rs/pull/724#discussion_r3941335814
##########
crates/core/tests/table_read_tests.rs:
##########
@@ -3477,3 +3477,143 @@ mod incremental_window_boundaries {
Ok(())
}
}
+
+mod hive_style_keygen_partition_pruning {
Review Comment:
I verified locally that all four tests in this module pass with the keygen
fix reverted to main: the v6 fixtures' hoodie.properties don't carry
`hoodie.keygen.timebased.timestamp.type` (0.15 sets it at write time but
doesn't persist it), so `TimestampBasedKeyGenerator::from_configs` errors, the
warn branch in partition.rs skips the transform, and these assertions are
satisfied by file-level column stats alone. Could you make the tests engage the
transform (e.g. `Table::new_with_options` with timestamp.type — which currently
surfaces the escaping issue I raised separately) or rewrite the comments to
describe what is actually pruning here? As written, e.g. "Before the reader
built hive paths correctly this returned every file instead" doesn't hold for
this fixture.
##########
crates/core/src/keygen/timestamp_based.rs:
##########
@@ -509,7 +482,13 @@ impl KeyGeneratorFilterTransformer for
TimestampBasedKeyGenerator {
let field = MetaField::PartitionPath.as_ref().to_string();
match filter.operator {
- ExprOperator::Eq | ExprOperator::Ne => {
+ // A negated predicate cannot prune, because this transform is
lossy: a
+ // partition holds every instant of its period, so `ts !=
2024-03-01T14:30:00Z`
+ // is satisfied by almost every row of `ts=2024-03-01`. Mapping
the negation
+ // onto the path would drop that whole partition and lose all of
them. Emit no
+ // partition filter and let the predicate be enforced per row.
+ ExprOperator::Ne | ExprOperator::NotIn => Ok(vec![]),
+ ExprOperator::Eq => {
let dt = self.parse_timestamp(&filter.values[0])?;
Review Comment:
When the transform does engage (1.x tables persist timestamp.type), a
filtered read currently fails outright: on the existing
v9_timebasedkeygen_nonhivestyle fixture, `ts_str >= '2023-11-14T00:00:00.000Z'`
errors with TimestampParsingError because `parse_data_for_options` keeps the
Java-properties escaping, so input.dateformat arrives as
`yyyy-MM-dd'T'HH\:mm\:ss.SSSZ`, and hoodie.properties overrides any
user-supplied value. That means the hive-style pruning fixed here has no
reachable end-to-end path yet — v6 tables skip the transform, v8/v9 tables
error on it. Would you consider unescaping in `parse_data_for_options`
(java.util.Properties semantics) in this PR, or filing a linked follow-up? The
integration tests can only pin the fixed behavior once this is resolved.
##########
crates/core/tests/gold_parity_tests.rs:
##########
@@ -85,6 +85,13 @@ const NULL_TOKEN: &str = "<null>";
/// being compared.
const EXPECTED_WITHOUT_GOLD: &[&str] = &[
"table_hfile_log_block [MorAvro]",
+ // Partition-pruning fixtures. They exist to pin the on-disk partition
path shape a
+ // hive-style timestamp or custom key generator produces, which is
asserted directly in
+ // table_read_tests, so they carry no Hudi read snapshot.
+ "v6_customkeygen_hivestyle [Cow]",
Review Comment:
non-blocking: v6_customkeygen_hivestyle and v9_customkeygen_hivestyle aren't
read by any test, so the "asserted directly in table_read_tests" part of this
comment only holds for the two timebased fixtures. Could you either add a small
smoke test pinning the custom-keygen layout (the v6 one is readable today) or
ship these two fixtures with the #549 change, and adjust the comment either way?
--
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]
