github-actions[bot] commented on code in PR #65998:
URL: https://github.com/apache/doris/pull/65998#discussion_r3643839162
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2332,28 +2300,17 @@ Status TableColumnMapper::localize_filters(const
std::vector<TableFilter>& table
// This keeps expression localization independent from filter iteration
order.
filter_mappings = _filter_visible_mappings();
const auto global_to_file_slot =
build_file_slot_rewrite_map(filter_mappings, _filter_entries);
- for (size_t filter_index = 0; filter_index < table_filters.size();
++filter_index) {
- const auto& table_filter = table_filters[filter_index];
+ for (const auto& table_filter : table_filters) {
if (table_filter.conjunct != nullptr && table_filter.conjunct->root()
!= nullptr) {
const auto root = table_filter.conjunct->root();
const auto impl = root->get_impl();
const auto predicate = impl != nullptr ? impl : root;
- if (!table_filter.can_localize || !predicate->is_deterministic() ||
+ if (!predicate->is_deterministic() ||
Review Comment:
**[P1] Preserve the suffix after an unsafe conjunct**
The safe-prefix boundary computed by `TableReader` no longer constrains this
localization loop. With `enable_adjust_conjunct_order_by_cost=false`, ordered
conjuncts such as `assert_true(part = 'expected', 'boom') AND id < 0`, a split
with `part = 'bad'`, and only nonnegative ids leave the unsafe partition
predicate at Scanner but localize `id < 0`; the FileReader removes every row,
so no row reaches Scanner and the expected `assert_true` error is never raised.
Once an original conjunct is unsafe to execute on selected rows, it and every
later conjunct must remain above the FileReader.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2332,28 +2300,17 @@ Status TableColumnMapper::localize_filters(const
std::vector<TableFilter>& table
// This keeps expression localization independent from filter iteration
order.
filter_mappings = _filter_visible_mappings();
const auto global_to_file_slot =
build_file_slot_rewrite_map(filter_mappings, _filter_entries);
- for (size_t filter_index = 0; filter_index < table_filters.size();
++filter_index) {
- const auto& table_filter = table_filters[filter_index];
+ for (const auto& table_filter : table_filters) {
if (table_filter.conjunct != nullptr && table_filter.conjunct->root()
!= nullptr) {
const auto root = table_filter.conjunct->root();
const auto impl = root->get_impl();
const auto predicate = impl != nullptr ? impl : root;
- if (!table_filter.can_localize || !predicate->is_deterministic() ||
+ if (!predicate->is_deterministic() ||
!table_filter_has_only_local_entries(table_filter,
_filter_entries)) {
continue;
}
- if (runtime_state != nullptr &&
-
runtime_state->query_options().truncate_char_or_varchar_columns &&
- std::ranges::any_of(table_filter.global_indices,
[&](GlobalIndex global_index) {
- const auto* mapping = _find_filter_mapping(global_index);
- return mapping != nullptr &&
requires_char_or_varchar_truncation(*mapping);
- })) {
- // The table predicate observes the bounded value after
finalize; evaluating it on
- // a wider file string would change equality and range
semantics.
- continue;
- }
- // FileReader becomes the exact owner only for a stable predicate
whose complete
- // expression can be rewritten against this split's physical
schema.
+ // Scanner evaluates the original conjunct after final
materialization. Only predicates
Review Comment:
**[P1] Keep width-changing string predicates above FileReader**
With `truncate_char_or_varchar_columns=true`, a table `VARCHAR(3)`, a wider
file value `abcd`, and `value = 'abc'`, this path evaluates the localized copy
against `abcd` before `TableReader::finalize_chunk()` truncates it to `abc`.
The Scanner safety-net runs too late to restore a row already discarded by the
FileReader (and ConditionCache can persist that false negative). Please retain
predicates for mappings that require CHAR/VARCHAR truncation above the
FileReader.
##########
be/test/format_v2/table_reader_test.cpp:
##########
@@ -1733,11 +1471,9 @@ TEST(TableReaderTest,
ConstantPruningStopsAtUnsafeSlotlessPredicate) {
EXPECT_EQ(fake_state->open_count, 1);
ASSERT_NE(fake_state->last_request, nullptr);
// A slotless unsafe conjunct is an ordering barrier even though it has no
TableFilter entry.
- // The later predicate must stay on the post-materialization path instead
of running inside the
+ // The later predicate must stay on the scanner's row-level path instead
of running inside the
Review Comment:
**[P1] Update the direct-TableReader expectations**
This comment correctly assigns the predicate to Scanner, but this test calls
`TableReader::get_block()` directly. The slotless predicate creates no
`TableFilter`, so after removing TableReader residual filtering nothing here
can set `predicate_executed`; the same stale `EXPECT_TRUE` remains in
`SlotlessConjunctDisablesAggregatePushdown` at line 1760. These focused BE
tests will fail unless they expect no TableReader execution or use a
`FileScannerV2` harness for the final conjunct.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -461,33 +450,18 @@ Status FileScannerV2::_get_block_impl(RuntimeState*
state, Block* block, bool* e
}
Status FileScannerV2::_filter_output_block(Block* block) {
- if (_scanner_residual_conjuncts.empty() || block->rows() == 0) {
- return Status::OK();
- }
- SCOPED_TIMER(_scanner_residual_filter_timer);
- const size_t rows_before_filter = block->rows();
- auto status = VExprContext::filter_block(_scanner_residual_conjuncts,
block, block->columns());
- if (!status.ok() && _params != nullptr &&
- _get_current_format_type() == TFileFormatType::FORMAT_ORC) {
- status.prepend("Orc row reader nextBatch failed. reason = ");
- }
- RETURN_IF_ERROR(status);
- const int64_t filtered_rows = cast_set<int64_t>(rows_before_filter -
block->rows());
- _counter.num_rows_unselected += filtered_rows;
- if (_scanner_residual_rows_filtered_counter != nullptr) {
- COUNTER_UPDATE(_scanner_residual_rows_filtered_counter, filtered_rows);
- }
- return Status::OK();
-}
-
-size_t FileScannerV2::_last_block_rows_read(const Block& block) const {
- const auto& stats = _table_reader->last_materialized_block_stats();
- return stats.has_materialized_input ? stats.rows : block.rows();
+ return
_contextualize_output_filter_status(Scanner::_filter_output_block(block),
Review Comment:
**[P1] Preserve predicate state and order when late RFs arrive**
This delegation makes a late-RF refresh use Scanner's fully recloned,
cost-sorted vector. With the default cost ordering, an RF such as `id >= 1` can
move ahead of an existing nonlocal `assert_true(part = 'expected', 'boom')`;
for a split with `part = 'bad'` and `id = -1`, the RF sets `can_filter_all`, so
`execute_conjuncts()` returns before the assertion and suppresses its expected
error. Recloning also reopens thread-local `FunctionContext` state, for example
reseeding `random(seed)`. Please preserve the existing Scanner contexts and
their established order, appending only the identity-tracked new RF contexts.
--
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]
