github-actions[bot] commented on code in PR #66343:
URL: https://github.com/apache/doris/pull/66343#discussion_r3694872688
##########
be/src/format_v2/parquet/reader/native/column_reader.cpp:
##########
@@ -1271,6 +1271,9 @@ Status ScalarColumnReader<IN_COLLECTION,
OFFSET_INDEX>::read_fixed_width_filter(
ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::
supports_raw_binary_filter_encoding(
encoding,
_chunk_meta.meta_data.type) ||
+ ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::
Review Comment:
[P1] This chunk-level whitelist is broader than the page-level predicate
capability. For a logical string backed by `FIXED_LEN_BYTE_ARRAY`, a comparison
can run only through the raw-binary consumer: PLAIN/BYTE_STREAM_SPLIT pages are
accepted, but RLE/PLAIN_DICTIONARY pages are not, and String SerDe has no
converted-fixed predicate path. Because this new clause makes metadata
containing both encodings pass the up-front check, a valid
PLAIN-first/dictionary-later chunk consumes the first page and then returns
`used_filter=false`; `NativeColumnReader` converts that late fallback into
corruption. Please reject dictionary encodings up front for raw-binary-only
conjuncts (or add a dictionary binary consumer), with a regression covering
that page order.
##########
be/src/format_v2/parquet/reader/native/fix_length_dict_decoder.hpp:
##########
@@ -39,6 +39,77 @@ class FixLengthDictDecoder final : public BaseDictDecoder {
static_cast<size_t>(_type_length));
}
+ Status decode_selected_fixed_values(const ParquetSelection& selection,
+ ParquetFixedValueConsumer& consumer)
override {
+ DORIS_CHECK_GT(_type_length, 0);
+ // Raw predicates on non-string dictionaries must observe decoded
physical values, not
+ // dictionary IDs, so expand each validated index before invoking the
predicate consumer.
+ class ExpandedValueConsumer final : public
ParquetDictionaryValueConsumer {
+ public:
+ ExpandedValueConsumer(const uint8_t* dictionary, size_t
dictionary_size,
+ size_t value_width,
ParquetFixedValueConsumer& consumer,
+ std::vector<uint8_t>& scratch)
+ : _dictionary(dictionary),
+ _dictionary_size(dictionary_size),
+ _value_width(value_width),
+ _consumer(consumer),
+ _scratch(scratch) {}
+
+ Status consume_indices(const uint32_t* indices, size_t num_values)
override {
+ DORIS_CHECK(indices != nullptr || num_values == 0);
+ if (UNLIKELY(num_values > std::numeric_limits<size_t>::max() /
_value_width)) {
+ return Status::IOError("Parquet dictionary expansion size
overflows");
+ }
+ _scratch.resize(num_values * _value_width);
+ for (size_t row = 0; row < num_values; ++row) {
+ DORIS_CHECK_LT(indices[row], _dictionary_size);
+ memcpy(_scratch.data() + row * _value_width,
+ _dictionary + static_cast<size_t>(indices[row]) *
_value_width,
+ _value_width);
+ }
+ return _consumer.consume(_scratch.data(), num_values,
_value_width);
+ }
+
+ Status consume_repeated(uint32_t index, size_t num_values)
override {
+ DORIS_CHECK_LT(index, _dictionary_size);
+ constexpr size_t MAX_BATCH_VALUES = 1024;
+ const uint8_t* value = _dictionary +
static_cast<size_t>(index) * _value_width;
+ while (num_values > 0) {
+ const size_t batch = std::min(num_values,
MAX_BATCH_VALUES);
+ _scratch.resize(batch * _value_width);
+ for (size_t row = 0; row < batch; ++row) {
+ memcpy(_scratch.data() + row * _value_width, value,
_value_width);
+ }
+ RETURN_IF_ERROR(_consumer.consume(_scratch.data(), batch,
_value_width));
+ num_values -= batch;
+ }
+ return Status::OK();
+ }
+
+ private:
+ const uint8_t* const _dictionary;
+ const size_t _dictionary_size;
+ const size_t _value_width;
+ ParquetFixedValueConsumer& _consumer;
+ std::vector<uint8_t>& _scratch;
+ } expanded_consumer(_dict.get(), _num_dictionary_values,
static_cast<size_t>(_type_length),
+ consumer, _expanded_values);
+ return decode_selected_dictionary_values(selection, expanded_consumer);
Review Comment:
[P2] This streams directly into `FixedWidthPredicateConsumer`, which appends
matching values to the caller-owned projected column for each validated
range/run. A later selected ID, filtered-tail ID, or consumer conversion can
still fail after those early appends, and neither this method nor
`filter_fixed_width_values()` restores the column. The ordinary dictionary
gather path explicitly snapshots and `resize(old_size)` on the same
late-corruption case (`DictionaryDirectGatherRollsBackLateCorruptRun`). Please
add the same all-or-nothing rollback around projected direct filtering and
cover a valid selected head followed by a corrupt filtered tail or later run.
##########
be/src/format_v2/parquet/reader/native/fix_length_dict_decoder.hpp:
##########
@@ -39,6 +39,77 @@ class FixLengthDictDecoder final : public BaseDictDecoder {
static_cast<size_t>(_type_length));
}
+ Status decode_selected_fixed_values(const ParquetSelection& selection,
+ ParquetFixedValueConsumer& consumer)
override {
+ DORIS_CHECK_GT(_type_length, 0);
+ // Raw predicates on non-string dictionaries must observe decoded
physical values, not
+ // dictionary IDs, so expand each validated index before invoking the
predicate consumer.
+ class ExpandedValueConsumer final : public
ParquetDictionaryValueConsumer {
+ public:
+ ExpandedValueConsumer(const uint8_t* dictionary, size_t
dictionary_size,
+ size_t value_width,
ParquetFixedValueConsumer& consumer,
+ std::vector<uint8_t>& scratch)
+ : _dictionary(dictionary),
+ _dictionary_size(dictionary_size),
+ _value_width(value_width),
+ _consumer(consumer),
+ _scratch(scratch) {}
+
+ Status consume_indices(const uint32_t* indices, size_t num_values)
override {
+ DORIS_CHECK(indices != nullptr || num_values == 0);
+ if (UNLIKELY(num_values > std::numeric_limits<size_t>::max() /
_value_width)) {
+ return Status::IOError("Parquet dictionary expansion size
overflows");
+ }
+ _scratch.resize(num_values * _value_width);
Review Comment:
[P1] This expansion is bounded by values, not bytes. `FIXED_LEN_BYTE_ARRAY`
accepts any positive `type_length`, and DECIMAL validation only checks that the
declared precision fits that width. A required DECIMAL(1,0) with a 1 MiB width,
a one-entry dictionary, and a 1,024-row repeated-ID page is only slightly over
1 MiB on disk, but `consume_repeated()` resizes this scratch to exactly 1 GiB
before Decimal SerDe rejects widths above `Int256` (or marks them NULL in
permissive mode). The fragmented/indices path can amplify further. Please
validate the logical/physical width before selecting this path and byte-bound
or chunk the expansion, with an oversized FLBA dictionary regression for strict
and permissive scans.
--
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]
