github-actions[bot] commented on code in PR #66263:
URL: https://github.com/apache/doris/pull/66263#discussion_r3680480331
##########
be/benchmark/parquet/AGENTS.md:
##########
@@ -299,7 +300,7 @@ be simulated by silently changing the local reader
benchmark.
## Current validation record
-The current expected registration counts are 228 decoder, 80 kernel, and 152
reader cases. A smoke
+The current expected registration counts are 228 decoder, 86 kernel, and 152
reader cases. A smoke
Review Comment:
[P2] Keep the reader registration count in sync
This refreshed validation record still says `ParquetReader` has 152 cases,
but `ReaderMatrixCoversNullableSparseAndProjectionAxes` now asserts
`reader_scenarios().size() == 167`, and `register_reader_benchmarks()`
registers every returned scenario. The list/count command above will therefore
report 167 and contradict this review guide (the stale 152 also appears in the
command comment and matrix description). Please update all three reader-count
references to 167 while refreshing these totals.
##########
be/benchmark/parquet/benchmark_parquet_kernels.hpp:
##########
@@ -34,6 +35,87 @@ namespace doris::parquet_benchmark {
namespace detail {
constexpr size_t KERNEL_ROWS = 1UL << 16;
+constexpr size_t NESTED_VALUES_PER_ROW = 8;
+
+inline void run_nested_selection_kernel(benchmark::State& state, const
KernelScenario& scenario) {
Review Comment:
[P2] Make the before/after result reproducible
This benchmark and the fused implementation are introduced together, so the
base revision cannot run `ParquetKernel/nested_selection` and the head exposes
only the new path. The summary table therefore cannot be reproduced without an
unsubmitted benchmark-only baseline. Per this directory's benchmark guide,
please provide the exact baseline patch/commit and command, compiler/CPU/cache
environment, and raw ABBA JSON (or retain selectable legacy and fused
implementations in the harness) so the claimed 1.79x-2.27x result can be
verified.
##########
be/benchmark/parquet/benchmark_parquet_kernels.hpp:
##########
@@ -34,6 +35,87 @@ namespace doris::parquet_benchmark {
namespace detail {
constexpr size_t KERNEL_ROWS = 1UL << 16;
+constexpr size_t NESTED_VALUES_PER_ROW = 8;
+
+inline void run_nested_selection_kernel(benchmark::State& state, const
KernelScenario& scenario) {
+ using format::parquet::native::ColumnSelectVector;
+ using format::parquet::native::FilterMap;
+ using format::parquet::native::level_t;
+
+ std::vector<level_t> source_repetition_levels;
+ std::vector<level_t> source_definition_levels;
+ source_repetition_levels.reserve(KERNEL_ROWS * NESTED_VALUES_PER_ROW);
+ source_definition_levels.reserve(KERNEL_ROWS * NESTED_VALUES_PER_ROW);
+ for (size_t row = 0; row < KERNEL_ROWS; ++row) {
+ if (row % 10 == 0) {
+ source_repetition_levels.push_back(0);
+ source_definition_levels.push_back(0);
+ continue;
+ }
+ for (size_t value = 0; value < NESTED_VALUES_PER_ROW; ++value) {
+ source_repetition_levels.push_back(value == 0 ? 0 : 1);
+ source_definition_levels.push_back((row + value) % 10 == 0 ? 2 :
3);
+ }
+ }
+
+ const auto parent_selection =
+ make_selection_plan(KERNEL_ROWS, scenario.selectivity_percent,
scenario.pattern);
+ std::vector<uint8_t> parent_filter_data(KERNEL_ROWS, 0);
+ visit_selected_rows(parent_selection,
+ [&](size_t row) { parent_filter_data[row] = uint8_t
{1}; });
+ FilterMap parent_filter;
+ auto status = parent_filter.init(parent_filter_data.data(),
parent_filter_data.size(), false);
+ if (!status.ok()) {
+ state.SkipWithError(status.to_string().c_str());
+ return;
+ }
+
+ std::vector<level_t> repetition_levels = source_repetition_levels;
+ std::vector<level_t> definition_levels = source_definition_levels;
+ ColumnSelectVector selection;
+ NullMap selected_nulls;
+ size_t ancestor_null_count = 0;
+ status = selection.init_nested(&repetition_levels, &definition_levels, 0,
+ /*repeated_parent_def_level=*/2,
+ /*definition_level=*/3, &selected_nulls,
&parent_filter, 0,
+ &ancestor_null_count);
+ if (!status.ok() || repetition_levels.size() != definition_levels.size() ||
Review Comment:
[P2] Validate nested-selection output before timing it
These checks cannot catch the kernel keeping the wrong parents or emitting
incorrect null/read runs: the two level vectors are resized together, and every
source entry necessarily contributes to either `num_values()` or
`ancestor_null_count`. The existing kernel benchmarks compare representative
output with an independent oracle before timing. Please build the expected
compacted repetition/definition levels, selected null map, filtered count, and
four-way read sequence independently and compare them here before publishing
timings.
##########
be/test/format_v2/parquet/parquet_reader_control_test.cpp:
##########
@@ -184,6 +185,80 @@ TEST(SelectionVectorTest,
IdentitySelectionDoesNotMaterializeFilter) {
EXPECT_EQ(filter, nullptr);
}
+TEST(NativeNestedSelectionTest,
BuildsSelectionAndCompactsSurvivingParentLevels) {
Review Comment:
[P1] Exercise the fused path through a real nested reader
These new tests call `init_nested()` directly, but the production change
also depends on `_read_nested_column()` supplying the right batch or
continuation filter index, `materialize_values()` and the ancestor-only skip
advancing the page cursor exactly, and ARRAY/MAP/STRUCT consumers rebuilding
the compacted shape. A partial-filter bug in any of those boundaries can shift
later values or corrupt offsets while both helper tests still pass. Please add
an end-to-end Parquet reader case with a partially filtered batch, selected and
rejected ancestor/leaf NULLs, a parent spanning pages, and follow-up output
that verifies exact values, offsets, and null maps (including sibling shape for
MAP/STRUCT).
--
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]
