github-actions[bot] commented on code in PR #65934:
URL: https://github.com/apache/doris/pull/65934#discussion_r3635335847
##########
be/benchmark/parquet/parquet_benchmark_scenarios.h:
##########
@@ -126,6 +126,19 @@ inline std::vector<ReaderScenario> reader_scenarios() {
scenario.operation = ReaderOperation::PREDICATE_SCAN;
add(scenario);
}
+ for (const auto encoding : {Encoding::BYTE_STREAM_SPLIT,
Encoding::DELTA_BINARY_PACKED}) {
Review Comment:
[P2] Update and assert the reader registration total
This loop contributes 14 unique registrations, not 16, because each
encoding's 10%-selectivity projected case already comes from the preceding
loop. The total therefore grows from 137 to 151, while
`be/benchmark/parquet/AGENTS.md` still tells smoke reviewers to expect 137 and
to reject a result with a different count. Please assert
`reader_scenarios().size() == 151` and update the current listing/matrix counts
(leaving the historical validation record tied to its old commit unchanged).
##########
be/src/format_v2/parquet/reader/native_column_reader.cpp:
##########
@@ -361,16 +363,17 @@ Status NativeColumnReader::read_with_plain_filter(int64_t
rows, const uint8_t* f
size_t loop_rows = 0;
IColumn::Filter loop_filter;
bool loop_used = false;
- RETURN_IF_ERROR(_native_reader->read_plain_filter(
- conjuncts, column_id, filter, static_cast<size_t>(rows -
*rows_read), &loop_filter,
- &loop_rows, &eof, &loop_used));
+ RETURN_IF_ERROR(_native_reader->read_fixed_width_filter(
Review Comment:
[P2] Keep the direct reader's count profile complete
`read_with_filter()` counts each native reader call in `NativeReadCalls`,
and both ordinary and dictionary `select()` paths add their input selection to
`ReaderSelectRows`. This fixed-width loop does neither, so a predicate-only
direct scan can report zero native calls/selected rows for its only active
reader; projected scans undercount them behind unrelated lazy-reader activity.
This is separate from the existing decode/materialization timer thread. Please
mirror both count updates here and assert them in the direct-path profile tests.
##########
be/src/format_v2/parquet/parquet_profile.cpp:
##########
@@ -175,10 +175,10 @@ void ParquetProfile::init(RuntimeProfile* profile) {
TUnit::BYTES,
parquet_profile, 1);
predicate_compaction_count = ADD_CHILD_COUNTER_WITH_LEVEL(profile,
"PredicateCompactionCount",
TUnit::UNIT,
parquet_profile, 1);
- plain_predicate_direct_batches = ADD_CHILD_COUNTER_WITH_LEVEL(
- profile, "PlainPredicateDirectBatches", TUnit::UNIT,
parquet_profile, 1);
- plain_predicate_direct_rows = ADD_CHILD_COUNTER_WITH_LEVEL(profile,
"PlainPredicateDirectRows",
- TUnit::UNIT,
parquet_profile, 1);
+ fixed_width_predicate_direct_batches = ADD_CHILD_COUNTER_WITH_LEVEL(
+ profile, "FixedWidthPredicateDirectBatches", TUnit::UNIT,
parquet_profile, 1);
Review Comment:
[P2] Preserve the profile keys or update their compatibility contract
This removes the only `PlainPredicateDirectBatches/Rows` registrations, but
the mandatory Parquet scan design still describes the predicate-only PLAIN path
and tells operators/profile parsers to read those exact keys. After this change
those lookups silently disappear, even for the existing PLAIN workload. Please
preserve the old keys as deprecated aliases (or retain them with broadened
semantics) and update the current-pipeline design to cover projected
PLAIN/BSS/Delta behavior and the new names.
##########
be/test/format_v2/parquet/native_decoder_test.cpp:
##########
@@ -175,6 +175,26 @@ TEST(ParquetV2NativeDecoderTest,
RawExprPreservesFloatNanOrdering) {
EXPECT_EQ(matches, (std::array<uint8_t, values.size()> {0, 0, 0, 1}));
}
+TEST(ParquetV2NativeDecoderTest,
RawFixedFilterSupportsIdentityWidthEncodingTypes) {
+ using Reader = ColumnChunkReader<false, false>;
+
EXPECT_TRUE(Reader::supports_raw_fixed_filter_encoding(tparquet::Encoding::BYTE_STREAM_SPLIT,
Review Comment:
[P2] Prove the newly accepted encoding/type matrix end to end
These assertions only test the whitelist. The new scan fixtures request
BSS/Delta from Arrow but never verify the emitted footer/page encoding, and
PLAIN increments the same direct counter, so both tests can stay green without
exercising the new decoders. They also cover only required INT32 with one
contiguous survivor run; the nullable benchmark checks row counts, not
projected values. Please add value-checked filter/project coverage for every
accepted BSS/Delta type (including nullable/fragmented and page transitions)
and assert the fixture's actual encoding before treating the benchmark numbers
as validation.
--
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]
