yihua commented on code in PR #748:
URL: https://github.com/apache/hudi-rs/pull/748#discussion_r3938961781
##########
crates/core/src/file_group/reader_v2/engine.rs:
##########
@@ -932,6 +932,63 @@ impl HoodieFileGroupReader {
present_len
);
+ // Parquet evaluates a pushed predicate against the file's PHYSICAL
values,
+ // before `project_batch_to_schema` runs. Sound only while a physical
value
+ // means what its physical type says, which the apache/hudi#18132
repair
+ // breaks: the file labels a tz-aware column micros while the stored
i64 is
+ // MILLIS, so a millis-semantics literal reads those rows as 1970 and
the
+ // filter drops rows that match. The post-scan filter cannot restore
them.
+ //
+ // Two gates, cheapest first. `repair_risk_columns` was decided ONCE
per scan
+ // from the table schema and the predicate's own referenced columns,
and is
+ // empty unless the predicate touches a tz-aware millis column — so the
+ // common scan never reaches the footer comparison below and never
loses
+ // pushdown. The footer schema itself is already fetched
unconditionally
+ // above, so gate 1 buys predicate scoping and the per-file name walk,
not
+ // avoided IO.
+ //
+ // The table side is `table_schema`, NOT `required_schema`: a filter
column
+ // absent from the projection is still decoded and still misread,
because a
+ // `RowFilter` builder derives its own `ProjectionMask` from the
parquet
+ // schema rather than from `intersection`.
+ let repair_conflict =
+ if pushdown_is_safe &&
!self.reader_context.repair_risk_columns.is_empty() {
+ let table_side = self
+ .schema_handler
+ .table_schema
+ .as_ref()
+ .unwrap_or(&required_schema);
Review Comment:
The `unwrap_or(&required_schema)` fallback quietly re-opens the
unprojected-column hole this guard closes: `build()` only populates
`table_schema` when `with_data_schema` was called, so an embedder using
`with_requested_schema` + `with_repair_risk_columns` gets the fallback, a
filter-only column absent from the projection resolves to nothing on the table
side, and `reinterpreted_columns` skips it — pushdown kept, rows dropped. Since
gate 1 only ever emits columns present in the real table schema, the
skip-on-table-absent arm in `reinterpreted_columns` is unreachable under
correct usage anyway — could it treat a candidate the table side cannot resolve
as a conflict instead, so the fallback fails toward losing pushdown rather than
losing rows?
##########
crates/core/src/file_group/reader_v2/engine.rs:
##########
@@ -1265,6 +1328,20 @@ impl HoodieFileGroupReaderBuilder {
self
}
+ /// Arm the value-reinterpreting repair guard with the predicate columns
the
+ /// apache/hudi#18132 logical-type repair could make a pushed filter
misread.
+ ///
+ /// Required alongside [`Self::with_row_filter_builder`] whenever the
table may
Review Comment:
The caveat that parquet log blocks stay unguarded (the `mor_pk_safe` gate in
`log_record_reader.rs` pushes the same builder with no repair check, so a
record key that is itself a mislabelled timestamp column is still exposed)
lives only in the PR description. Could you note it here or on the log-block
gate's comment, so the residual exposure is visible next to the code that
carries it?
##########
crates/core/src/file_group/reader_v2/engine.rs:
##########
@@ -932,6 +932,63 @@ impl HoodieFileGroupReader {
present_len
);
+ // Parquet evaluates a pushed predicate against the file's PHYSICAL
values,
+ // before `project_batch_to_schema` runs. Sound only while a physical
value
+ // means what its physical type says, which the apache/hudi#18132
repair
+ // breaks: the file labels a tz-aware column micros while the stored
i64 is
+ // MILLIS, so a millis-semantics literal reads those rows as 1970 and
the
+ // filter drops rows that match. The post-scan filter cannot restore
them.
+ //
+ // Two gates, cheapest first. `repair_risk_columns` was decided ONCE
per scan
+ // from the table schema and the predicate's own referenced columns,
and is
+ // empty unless the predicate touches a tz-aware millis column — so the
+ // common scan never reaches the footer comparison below and never
loses
+ // pushdown. The footer schema itself is already fetched
unconditionally
+ // above, so gate 1 buys predicate scoping and the per-file name walk,
not
+ // avoided IO.
+ //
+ // The table side is `table_schema`, NOT `required_schema`: a filter
column
+ // absent from the projection is still decoded and still misread,
because a
+ // `RowFilter` builder derives its own `ProjectionMask` from the
parquet
+ // schema rather than from `intersection`.
+ let repair_conflict =
+ if pushdown_is_safe &&
!self.reader_context.repair_risk_columns.is_empty() {
+ let table_side = self
+ .schema_handler
+ .table_schema
+ .as_ref()
+ .unwrap_or(&required_schema);
+ crate::schema::batch_evolution::reinterpreted_columns(
+ &file_schema,
+ table_side,
+ &self.reader_context.repair_risk_columns,
+ )?
+ } else {
+ Vec::new()
+ };
+
+ // ONE verdict, both consumers, withdrawn in one block. That is the
same
+ // property the merge-safety gate is bound once for, one layer in: an
edit
+ // to the condition cannot leave the row-group selector behind, and
pruning
+ // is the one that must not be left behind — it drops rows before
anything
+ // downstream can see them.
+ if !repair_conflict.is_empty() {
Review Comment:
This block runs even when neither a row filter nor a selector was installed
(only `repair_risk_columns` is set), so `pushdown_suppressed_by_repair` can
count a withdrawal that never happened and the debug line claims to skip
pushdown that was never requested. Would it make sense to gate the
`repair_conflict` computation on `row_filter.is_some() ||
row_group_selector.is_some()`, so the counter stays faithful and the footer
walk is skipped when there is nothing to withdraw?
--
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]
