github-actions[bot] commented on code in PR #66360:
URL: https://github.com/apache/doris/pull/66360#discussion_r3701283477
##########
be/src/format_v2/parquet/selection_vector.h:
##########
@@ -173,19 +197,65 @@ class SelectionVector {
}
previous = current;
}
+ if (!_mutable_data_exposed) {
+ _verified_generation = _generation;
+ _verified_count = count;
+ _verified_batch_rows = batch_rows;
+ }
return Status::OK();
}
private:
+ void _materialize_identity() {
+ if (_data != nullptr) {
+ return;
+ }
+ _owned.resize(_size);
+ _data = _owned.data();
+ for (size_t idx = 0; idx < _size; ++idx) {
+ _data[idx] = static_cast<Index>(idx);
+ }
+ }
+
+ size_t _compact(const uint8_t* filter, size_t count, bool
filter_uses_row_index) {
+ DORIS_CHECK(filter != nullptr);
+ DORIS_CHECK(count <= _size);
+ Index* source = _data;
+ if (_data == nullptr) {
+ _owned.resize(_size);
Review Comment:
[P2] Retain the owned selection scratch across batch resets. Every request
with row/delete conjuncts calls `resize(batch_rows)`, which now clears
`_owned`; the first rejecting predicate then executes `_owned.resize(_size)`
and value-initializes the full index buffer before overwriting survivors. This
is the repeated cost behind the PR's own roughly 30-61%
dense/successive-compaction regressions, and production pays it for every
filtered batch. Please keep identity implicit via `_data == nullptr` while
retaining a high-water owned buffer (growing it lazily only when needed), then
rerun the named selection cases and a predicate-reader case.
##########
be/test/format_v2/parquet/parquet_scan_test.cpp:
##########
@@ -3163,17 +3179,82 @@ TEST_F(ParquetScanTest,
PredicateOnlyDictionaryRangeSkipsTypedValueMaterializati
conjunct->close();
}
+TEST_F(ParquetScanTest, DictionaryFiltersAreBuiltFromEachReaderSnapshot) {
+ struct ScanResult {
+ std::vector<int32_t> scores;
+ int64_t typed_compare_columns = 0;
+ };
+
+ auto scan = [&](int32_t lower_bound) {
+ RuntimeProfile profile("profile");
+ RuntimeState state {TQueryOptions(), TQueryGlobals()};
+ auto reader = create_reader(0, -1, &profile);
+ EXPECT_TRUE(reader->init(&state).ok());
+
+ std::vector<format::ColumnDefinition> schema;
+ EXPECT_TRUE(reader->get_schema(&schema).ok());
+ auto request = std::make_shared<format::FileScanRequest>();
+ format::FileScanRequestBuilder request_builder(request.get());
+
EXPECT_TRUE(request_builder.add_predicate_column(format::LocalColumnId(0)).ok());
+
EXPECT_TRUE(request_builder.add_non_predicate_column(format::LocalColumnId(1)).ok());
+ request->predicate_only_columns.push_back(format::LocalColumnId(0));
+ auto conjunct =
+ create_int32_function_conjunct(0, "gt", TExprOpcode::GT,
lower_bound, false);
+ EXPECT_TRUE(conjunct->prepare(&state, RowDescriptor()).ok());
+ EXPECT_TRUE(conjunct->open(&state).ok());
+ request->conjuncts.push_back(conjunct);
+ EXPECT_TRUE(reader->open(request).ok());
+
+ ScanResult result;
+ bool eof = false;
+ while (!eof) {
+ Block block = build_file_block(schema);
+ size_t rows = 0;
+ EXPECT_TRUE(reader->get_block(&block, &rows, &eof).ok());
+ const auto& score_column =
int32_data_column(*block.get_by_position(1).column);
+ for (size_t row = 0; row < rows; ++row) {
+ result.scores.push_back(score_column.get_element(row));
+ }
+ }
+ result.typed_compare_columns = counter_value(profile,
"DictFilterTypedCompareColumns");
+ conjunct->close();
+ EXPECT_TRUE(reader->close().ok());
+ return result;
+ };
+
+ write_dictionary_int_pair_parquet_file(_file_path);
+ const auto first = scan(2);
+ EXPECT_EQ(first.scores, std::vector<int32_t>({30, 40, 50, 60}));
+ EXPECT_EQ(first.typed_compare_columns, 1);
+
+ const auto repeated = scan(2);
+ EXPECT_EQ(repeated.scores, first.scores);
+ EXPECT_EQ(repeated.typed_compare_columns, 1);
+
+ const auto changed_predicate = scan(3);
+ EXPECT_EQ(changed_predicate.scores, std::vector<int32_t>({40, 50, 60}));
+ EXPECT_EQ(changed_predicate.typed_compare_columns, 1);
+
+ write_dictionary_int_pair_parquet_file(_file_path, {1, 2, 7, 8, 9, 10});
Review Comment:
[P2] Make this replacement dictionary change the matching entry-ID bitmap.
Both `[1,2,3,4,5,6]` and `[1,2,7,8,9,10]` under `id > 2` keep IDs 2-5 and
return the same score positions, so an implementation that reuses the first
dictionary's filter bitmap still passes the branch intended to catch that bug.
Reorder values across the threshold (for example `[7,1,8,2,9,3]`) and assert
the resulting different score positions.
##########
be/benchmark/parquet/AGENTS.md:
##########
@@ -46,7 +48,10 @@ be/output/lib/benchmark_test --benchmark_list_tests \
| grep -c '^ParquetKernel/' # currently 80
be/output/lib/benchmark_test --benchmark_list_tests \
- | grep -c '^ParquetReader/' # currently 152
+ | grep -c '^ParquetSelection/' # currently 25
+
+be/output/lib/benchmark_test --benchmark_list_tests \
+ | grep -c '^ParquetReader/' # currently 167
Review Comment:
[P3] Please update the later `Current validation record` too. This section
and the invariant tests now require 228 decoder, 80 kernel, 25 selection, and
167 reader cases, but line 319 still omits Selection and says 152 Reader cases.
Leaving two authoritative counts in this mandatory guide makes a correct smoke
listing look invalid.
--
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]
