github-actions[bot] commented on code in PR #65965:
URL: https://github.com/apache/doris/pull/65965#discussion_r3639110095
##########
be/benchmark/parquet/benchmark_parquet_reader.hpp:
##########
@@ -275,6 +322,22 @@ inline VExprContextSPtr make_predicate(int
column_position, int selectivity_perc
return context;
}
+inline VExprContextSPtr make_complex_residual_predicate(int
selectivity_percent) {
+ TExprNode node;
+ node.__set_node_type(TExprNodeType::COMPOUND_PRED);
+ node.__set_opcode(TExprOpcode::COMPOUND_AND);
+ node.__set_type(std::make_shared<DataTypeUInt8>()->to_thrift());
+ node.__set_num_children(2);
+ node.__set_is_nullable(false);
+ auto compound = VCompoundPred::create_shared(node);
+ compound->add_child(std::make_shared<Int32PairSumLessThanExpr>(0, 1, 2 *
selectivity_percent));
Review Comment:
[P2] Exercise a production-eligible expression
This custom leaf inherits `VExpr`'s safe result because it has no children,
but a real planned `(c0 + c1) < bound` tree contains a
`VectorizedFnCall("add")`, which is absent from the recursive selected-row
allowlist. Production therefore takes the eager fallback and reads all
predicate columns, so this benchmark and the matching tests do not exercise the
advertised workload. Please construct the production expression tree and assert
its counters; if arithmetic is intentionally unsafe, use and document a
genuinely reachable multi-column predicate instead.
##########
be/benchmark/parquet/AGENTS.md:
##########
@@ -42,7 +42,7 @@ be/output/lib/benchmark_test --benchmark_list_tests \
| grep -c '^ParquetDecoder/' # currently 152
be/output/lib/benchmark_test --benchmark_list_tests \
- | grep -c '^ParquetReader/' # currently 137
+ | grep -c '^ParquetReader/' # currently 138
Review Comment:
[P2] Correct the reader registration count
This smoke-check total is still stale. Deduplicating the exact generator
tuples produces 152 registrations (6 operation baselines + 6 encoding + 14
fixed-width-axis + 7 width/position + 119
nullable/pattern/selectivity/projection), not 138. A healthy full reader run
would therefore disagree with the documented check. Please update both count
references and add an exact scenario-count/unique-name assertion so this gate
cannot drift again.
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1875,15 +1986,45 @@ Status
ParquetScanScheduler::read_filter_columns(int64_t batch_rows,
};
RETURN_IF_ERROR(read_round_by_round());
- // Single-column expressions only touch the just-read column, so earlier
columns can retain
- // their own row mappings. Compact only when a later expression needs a
shared coordinate
- // space; otherwise the final boundary can discard hidden predicate
payloads without scanning
- // them again.
- if (!schedule.remaining_conjuncts.empty()) {
+ if (*selected_rows == 0) {
+ RETURN_IF_ERROR(skip_unmaterialized_predicate_columns());
+ return compact_predicate_columns_with_profile(true);
+ }
+
+ // Complex residuals keep their original conjunct order. Materialize only
the columns needed
+ // by the next reachable expression, then compact previously read columns
into the same row
+ // space before evaluating it. This is the scanner-side equivalent of
expression-triggered
+ // lazy columns: a conjunct that rejects the batch prevents later-only
columns from decoding.
+ for (const auto& stage : schedule.remaining_stages) {
Review Comment:
[P1] Avoid quadratic stage-alignment work
This loop revalidates every previously materialized predicate column before
every stage, even when the preceding stage left `SelectionVector` unchanged.
For N all-pass safe pair predicates (`c0 < c1`, `c2 < c3`, ...), stage i scans
2i full row mappings, so the added alignment overhead is O(N^2 * batch_rows):
661,903,500 comparisons for 100 pairs at a 65,535-row batch, before expression
work. Please track the selection generation or dirty state and skip alignment
when it has not changed, with an all-pass chain test or benchmark that protects
linear scaling.
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1220,18 +1278,31 @@ detail::PredicateConjunctSchedule
build_predicate_conjunct_schedule(
// optimization, so any unsafe conjunct disables the per-column
schedule for the batch.
schedule.remaining_conjuncts = request.conjuncts;
schedule.single_column_conjuncts.clear();
+ schedule.remaining_stages.clear();
+ schedule.supports_lazy_materialization = false;
return schedule;
}
std::set<int> referenced_positions;
conjunct->root()->collect_slot_column_ids(referenced_positions);
if (referenced_positions.size() != 1) {
schedule.remaining_conjuncts.push_back(conjunct);
+ if (!append_residual_stages(conjunct, conjunct->root(),
predicate_block_positions,
Review Comment:
[P1] Preserve localized CAST error behavior
The safety check here runs after ColumnMapper has cloned an ordinary
`VCastExpr` into `format::Cast`. Unlike `VCastExpr` (explicitly unsafe; only
`TryCastExpr` opts back in), `format::Cast` has no safety override and inherits
a true result for a deterministic slot child. A localized compound such as `(c0
< c1) AND (CAST(c2 AS table_type) = literal)` now filters on the first child
and runs the potentially failing cast only on survivors, hiding an error in a
rejected row that the former whole-root execution evaluated. Please preserve or
recheck ordinary-cast safety after localization (for example, make
`format::Cast` unsafe) and add a partial-survival failing-cast test.
--
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]
