github-actions[bot] commented on code in PR #66056:
URL: https://github.com/apache/doris/pull/66056#discussion_r3656209102
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1644,8 +1644,10 @@ bool ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::can_filter_fixed_width_valu
}
const auto primitive_type =
remove_nullable(_field_schema->data_type)->get_primitive_type();
const bool has_identity_width =
- (_metadata.type == tparquet::Type::INT32 && primitive_type ==
TYPE_INT) ||
- (_metadata.type == tparquet::Type::INT64 && primitive_type ==
TYPE_BIGINT) ||
+ (_metadata.type == tparquet::Type::INT32 &&
+ (primitive_type == TYPE_INT || primitive_type == TYPE_DECIMAL32))
||
Review Comment:
[P1] Preserve decimal conversion before raw filtering
These new DECIMAL32/64 branches treat matching byte width as identity, but
the ordinary decimal SerDe still has required work: Parquet DECIMAL(p<=9/18)
uses INT32/INT64 carriers whose full 10/19-digit domains do not fit the
declared precision. `DecimalParquetConsumer::append_integers()` therefore
validates every carrier and either raises `DataQualityError` or marks a
nullable value NULL. The raw path can instead reject the carrier before SerDe
(so a strict predicate-only scan silently succeeds), or copy a
surviving/Bloom-false-positive carrier with `insert_many_raw_data()` as a
non-NULL decimal. For example, an INT32 carrier `999999` in DECIMAL(5,0) no
longer has the ordinary error behavior. Please keep these decimals on the SerDe
path, or perform the same precision/scale and strict/non-strict conversion
before treating the raw bitmap/projection as final; cover malformed carriers in
both predicate-only and projected tests.
##########
be/src/exprs/vdirect_in_predicate.h:
##########
@@ -98,6 +98,54 @@ class VDirectInPredicate final : public VExpr {
std::dynamic_pointer_cast<VSlotRef>(get_child(0)) != nullptr;
}
+ ZoneMapFilterResult evaluate_dictionary_filter(
+ const DictionaryEvalContext& ctx) const override {
+ return expr_zonemap::eval_in_dictionary(ctx, get_child(0), false,
_seg_filter_values);
Review Comment:
[P1] Avoid quadratic string-IN dictionary probing
This newly enables string Direct-IN runtime filters to call
`eval_in_dictionary()`, which iterates every materialized IN value and uses
`dictionary_contains()` to linearly scan the supplied dictionary. Parquet first
does that against the full row-group dictionary, then its generic ID-bitmap
builder supplies one dictionary entry at a time and repeats the entire IN list
for every ID because the typed string kernel recognizes only
`VectorizedFnCall`, not this runtime-filter wrapper. That makes setup
O(dictionary entries * RF values): a 65K-entry dictionary and 1K-value filter
already perform about 65 million `Field` comparisons per stage, while the
configured IN limit is much larger and the underlying `HybridSet` already
supports hash probes. Please add a wrapper-aware string kernel that probes the
HybridSet once per dictionary entry (or disable this delegation for strings),
with a large-cardinality regression test/benchmark.
##########
be/src/exprs/runtime_filter_expr.cpp:
##########
@@ -236,6 +236,37 @@ bool RuntimeFilterExpr::can_evaluate_zonemap_filter()
const {
return _impl->can_evaluate_zonemap_filter();
}
+bool RuntimeFilterExpr::can_execute_on_raw_fixed_values(const DataTypePtr&
data_type,
+ int column_id) const {
+ // Raw and dictionary streams omit NULL payloads and currently map NULL
rows to false. A
+ // null-aware RF must therefore stay on execute_filter(), which restores
its NULL semantics.
+ return !_null_aware && _impl->can_execute_on_raw_fixed_values(data_type,
column_id);
+}
+
+Status RuntimeFilterExpr::execute_on_raw_fixed_values(const uint8_t* values,
size_t num_values,
+ size_t value_width,
+ const DataTypePtr&
data_type, int column_id,
+ uint8_t* matches) const {
+ if (!can_execute_on_raw_fixed_values(data_type, column_id)) {
+ return Status::NotSupported("Runtime filter {} cannot evaluate raw
fixed-width values",
+ _filter_id);
+ }
+ return _impl->execute_on_raw_fixed_values(values, num_values, value_width,
data_type, column_id,
Review Comment:
[P2] Preserve runtime-filter sampling and accounting
The raw delegate here (and the dictionary delegate below) calls `_impl`
directly, bypassing `RuntimeFilterExpr::execute_filter()`, which is the only
place that consults `maybe_always_true_can_ignore()`, updates
`RuntimeFilterSelectivity`, and increments the per-filter
input/rejected/always-true counters. Parquet treats these bitmaps as final and
removes the exact residual, so rows rejected early never reach the wrapper. A
six-row RF that removes three can consequently be profiled/sampled as three
inputs and zero rejects; ineffective filters also cannot age into the ignore
state, and aggregate Parquet counters cannot identify which RF did the work.
Please plumb the owning wrapper's accounting/ignore state into bitmap
application (or retain an accounting-only residual), and assert the RF-specific
counters in the new scan tests.
##########
be/src/format_v2/parquet/reader/native/decoder.h:
##########
@@ -246,6 +256,57 @@ class BaseDictDecoder : public Decoder {
size_t active_scratch_bytes() const override { return _skip_indices.size()
* sizeof(uint32_t); }
protected:
+ static bool _is_fragmented_selection(const ParquetSelection& selection) {
+ constexpr size_t MIN_FRAGMENTED_RANGES = 8;
+ constexpr size_t MAX_AVERAGE_RANGE_VALUES = 4;
+ constexpr size_t MAX_DECODE_EXPANSION = 8;
+ return selection.ranges.size() >= MIN_FRAGMENTED_RANGES &&
selection.selected_values != 0 &&
+ selection.total_values / selection.selected_values <=
MAX_DECODE_EXPANSION &&
+ selection.selected_values <= selection.ranges.size() *
MAX_AVERAGE_RANGE_VALUES;
+ }
+
+ Status _decode_fragmented_selection(const ParquetSelection& selection,
+ size_t num_dictionary_values) {
+ // Decode and validate the page batch once when predicate survivors
alternate in tiny runs.
+ // Walking each range separately turns one RLE batch into millions of
decoder calls for
+ // low-cardinality predicates such as TPC-DS quantity buckets.
+ _skip_indices.resize(selection.total_values);
Review Comment:
[P2] Clear fragmented scratch's active size after compaction
This resizes `_skip_indices` to the full physical batch and compacts
survivors into its prefix, but never reduces the vector's logical size. A valid
65,535-parent nested batch alternating four selected leaf IDs with 31 rejected
IDs has 32,768 ranges, 131,072 selected IDs, and 1,146,849 total IDs (4,587,396
bytes); it passes the new gates because integer `total / selected == 8` and
`selected == 4 * ranges`. If later dictionary pages are small all-repeated RLE
runs, neither `_decode_dictionary_values()` nor `set_data()` resizes this
vector. `active_scratch_bytes()` therefore remains above the 4 MiB limit even
though those IDs are dead, and `release_batch_scratch()` resets the idle
counter forever instead of releasing the capacity. Please reset/shrink the
logical active size after the fragmented result is consumed, and add a
high-water test that follows such an outlier with repeated-run idle batches
through the release hysteresis.
--
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]
