github-actions[bot] commented on code in PR #65500:
URL: https://github.com/apache/doris/pull/65500#discussion_r3567835356
##########
be/src/format_v2/table_reader.h:
##########
@@ -900,6 +919,14 @@ class TableReader {
if (agg_type != TPushAggOp::type::COUNT && agg_type !=
TPushAggOp::type::MINMAX) {
return false;
}
+ // Aggregate pushdown returns reduced synthetic rows and may close the
physical reader
+ // before the next scheduler turn. If a runtime filter is still
pending, those rows could
+ // escape before the filter arrives and cannot later be reconstructed
from real file rows.
+ // This is the same irreversibility constraint as table-level metadata
COUNT, and applies
+ // to COUNT and MIN/MAX for Parquet/ORC as well as COUNT for text
readers.
+ if (!_all_runtime_filters_applied_for_split) {
Review Comment:
This still allows aggregate pushdown to reduce rows before an original
row-level predicate has run. `_build_table_filters_from_conjuncts()`
intentionally omits slotless conjuncts, so a scan with `MIN/MAX(v)` and a
slotless unsafe predicate like `random()`/`uuid()` can reach this new RF gate
with no pending filters and then pass the later `_table_filters.empty()` check.
The reader then emits synthetic extrema rows and closes before
`Scanner::_filter_output_block()` evaluates the original predicate, so the
predicate runs on `{min,max}` instead of on every source row. For rows
`{1,2,3}`, if the slotless predicate keeps only the middle row, normal scanning
aggregates `2`, but pushdown can aggregate `{1,3}`. Please disable aggregate
pushdown whenever the original `_conjuncts` is non-empty, or otherwise prove
every original conjunct has been represented by a safe, row-preserving
pre-execution path before emitting synthetic COUNT/MIN/MAX rows.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -540,6 +606,16 @@ static VExprSPtr rewrite_literal_to_file_type(const
VExprSPtr& literal_expr,
if (rewrite_info.file_type->equals(*original_literal->data_type())) {
return original_literal;
}
+ // A literal round trip alone cannot prove that file-local evaluation is
safe: the file slot
+ // itself may lose information when materialized as the table type. For
example, DOUBLE 1.5
+ // becomes BIGINT 1, so table predicate `value = 1` is true while file
predicate
+ // `value = 1.0` is false. Complex Field equality also does not compare
nested contents.
+ // Restrict localization to scalar numeric casts that preserve every file
value; unsupported
+ // and complex casts keep the table predicate and evaluate after
materialization.
+ if (!is_lossless_file_to_table_numeric_cast(rewrite_info.file_type,
Review Comment:
This numeric-only guard leaves TIMESTAMPTZ scale-mismatch predicates to the
generic slot rewrite path, which then wraps the file slot in `Cast(file_slot AS
table_type)`. That conflicts with the mapping contract added for TIMESTAMPTZ
scale mismatch: finalization treats TIMESTAMPTZ(6) -> TIMESTAMPTZ(0) as
pass-through because the SQL cast rounds fractional seconds. For example, a
file value `12:34:56.789` and predicate `ts < '12:34:57'` should remain true
under pass-through semantics, but a pushed file-reader cast to TIMESTAMPTZ(0)
can round the value to `12:34:57` and filter it before the scanner evaluates
the original predicate. Please keep these scale-mismatch TIMESTAMPTZ filters
above the file reader, or add a no-rounding file-local rewrite that matches
final materialization semantics.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -990,6 +1078,18 @@ static bool mapping_can_use_file_column_directly(const
ColumnMapping& mapping) {
return !needs_complex_rematerialize(mapping);
}
+static FilterConversionType direct_filter_conversion(const ColumnMapping&
mapping) {
+ DORIS_CHECK(mapping.table_type != nullptr);
+ // FileScanOperator deliberately keeps VARBINARY predicates above external
readers. Their
+ // physical binary representations are not uniformly supported by
reader-side expression and
+ // metadata filtering, so localizing a late runtime filter here can
incorrectly reject rows.
+ if (remove_nullable(mapping.table_type)->get_primitive_type() ==
TYPE_VARBINARY) {
Review Comment:
This guard only protects top-level VARBINARY mappings. Predicates rooted at
a complex column are admitted by `localize_filters()` based on the root
mapping, so nested VARBINARY can still reach the file reader. For example,
`element_at(s, 'bin') = ...` rewrites the struct selector/type to the file
child without checking the child mapping's `filter_conversion`; similarly,
generic complex-root predicates such as `array_contains(array<varbinary>, ...)`
or `map_contains_key(map<varbinary,...>, ...)` rewrite the root slot directly
when the root mapping has a local source. That sends nested VARBINARY
comparisons into the same file-reader expression/metadata paths this guard is
trying to avoid for top-level VARBINARY. Please reject localization for any
complex-root predicate whose referenced nested mapping is `FINALIZE_ONLY`, or
at least whose nested table type contains VARBINARY.
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -524,13 +525,25 @@ bool set_timestamp_zone_map(const
::orc::ColumnStatistics& statistics,
!timestamp_statistics->hasMaximum()) {
return false;
}
+ const auto min_endpoint =
+ std::pair(timestamp_statistics->getMinimum(),
timestamp_statistics->getMinimumNanos());
+ const auto max_endpoint =
+ std::pair(timestamp_statistics->getMaximum(),
timestamp_statistics->getMaximumNanos());
+ if (min_endpoint > max_endpoint) {
Review Comment:
This adds conservative validation for timestamp stats only, but ORC MIN/MAX
pushdown also consumes the sibling non-timestamp stripe stats through
`build_zone_map_from_orc_statistics()`. The integer, floating, string, date,
and decimal setters still return usable ZoneMaps without checking `max_value <
min_value`, and the floating path also trusts NaN bounds. With corrupt ORC
stats such as INT min=10/max=1 for a stripe that actually contains 5, aggregate
pushdown can return synthetic extrema `{10,1}` instead of falling back to a
real scan. Please apply the same conservative validation to every ORC
statistics-backed min/max path, including rejecting NaN floating bounds and
decoded inverted ranges, before allowing pruning or aggregate pushdown to trust
the stats.
--
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]
