github-actions[bot] commented on code in PR #66575:
URL: https://github.com/apache/doris/pull/66575#discussion_r3740278847
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1129,6 +1154,22 @@ Status ParquetScanScheduler::open_next_row_group(
const auto& row_group_metadata =
file_context.native_metadata->to_thrift().row_groups[row_group_idx];
+ _current_row_group_request = std::make_unique<format::FileScanRequest>();
+ _current_row_group_request->predicate_columns = request.predicate_columns;
+ _current_row_group_request->non_predicate_columns =
request.non_predicate_columns;
+ _current_row_group_request->count_star_placeholder_columns =
+ request.count_star_placeholder_columns;
+ VariantRowGroupProjectionCounts variant_projection_counts;
+ if (file_context.contains_variant) {
+ const auto predicate_counts =
finalize_variant_projections_for_row_group(
Review Comment:
[P2] Finalize the physical projection before row-group planning
`finalize_native_row_group_read_plan()` has already run with the leaf
candidate before this fallback is applied. If this row group expands to the
full Variant wrapper, newly added leaves are absent from the loaded
OffsetIndexes, so their readers take the non-OffsetIndex path and walk page
gaps even when page pruning produced sparse `selected_ranges`. The same
ordering also makes a group pruned before reaching this line account
`FilteredBytes` from the leaf candidate instead of the full physical request.
Please build the row-group-local physical columns before pruning/page-index
planning and use that shape for requested leaf IDs and avoided-byte accounting,
while keeping the immutable logical request for conjunct semantics.
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -701,11 +701,15 @@ std::optional<ResolvedVariantShredding>
resolve_variant_shredding(
if (wrapper == nullptr || wrapper->kind !=
ParquetColumnSchemaKind::VARIANT) {
return std::nullopt;
}
+ std::vector<const ParquetColumnSchema*> fallback_values;
for (const auto& component : predicate.path) {
+ const auto* fallback = child_named(*wrapper, "value");
const auto* typed_object = child_named(*wrapper, "typed_value");
- if (typed_object == nullptr || typed_object->kind !=
ParquetColumnSchemaKind::STRUCT) {
+ if (fallback == nullptr || fallback->kind !=
ParquetColumnSchemaKind::PRIMITIVE ||
+ typed_object == nullptr || typed_object->kind !=
ParquetColumnSchemaKind::STRUCT) {
return std::nullopt;
}
+ fallback_values.push_back(fallback);
Review Comment:
[P2] Do not gate leaf statistics on unrelated ancestor overflow
The [Variant shredding
contract](https://parquet.apache.org/docs/file-format/types/variantshredding/)
makes a partially shredded object's `value` keys disjoint from the fields
represented by its `typed_value`. Consequently a root/ancestor residual may
legitimately be non-null because it contains unrelated fields while this
requested descendant is still completely represented by its own wrapper. Adding
every ancestor here changes even a shallow `v['n']` predicate from checking
`n.value` to also requiring the root residual to be empty, which disables
row-group and page-index pruning for valid rows such as `{n: 5, extra: 7}`.
Please keep the all-null proof on the requested field's corresponding `value`
column; ancestor overflow cannot shadow that shredded key in a conforming
Parquet Variant.
##########
be/test/format_v2/parquet/parquet_reader_test.cpp:
##########
@@ -78,13 +79,65 @@
#include "storage/index/zone_map/zonemap_filter_result.h"
#include "storage/segment/condition_cache.h"
#include "storage/utils.h"
+#include "util/coding.h"
#include "util/defer_op.h"
+#include "util/thrift_util.h"
namespace doris {
namespace {
constexpr int64_t ROW_COUNT = 5;
+void duplicate_variant_fixture_with_unsafe_second_row_group(const std::string&
source_path,
+ const std::string&
output_path) {
+ std::filesystem::copy_file(source_path, output_path,
+
std::filesystem::copy_options::overwrite_existing);
+ std::ifstream input(output_path, std::ios::binary | std::ios::ate);
+ DORIS_CHECK(input.good());
+ const auto input_size = static_cast<std::streamoff>(input.tellg());
+ DORIS_CHECK(input_size >= static_cast<std::streamoff>(8));
+ std::vector<uint8_t> file_bytes(cast_set<size_t>(input_size));
+ input.seekg(0);
+ input.read(reinterpret_cast<char*>(file_bytes.data()),
cast_set<std::streamsize>(input_size));
+ DORIS_CHECK(input.good());
+ DORIS_CHECK(memcmp(file_bytes.data() + file_bytes.size() - 4, "PAR1", 4)
== 0);
+
+ const uint32_t footer_size = decode_fixed32_le(file_bytes.data() +
file_bytes.size() - 8);
+ DORIS_CHECK(footer_size <= file_bytes.size() - 8);
+ const size_t footer_offset = file_bytes.size() - 8 - footer_size;
+ uint32_t thrift_size = footer_size;
+ tparquet::FileMetaData metadata;
+ DORIS_CHECK(
+ deserialize_thrift_msg(file_bytes.data() + footer_offset,
&thrift_size, true, &metadata)
+ .ok());
+ DORIS_CHECK(metadata.row_groups.size() == 1);
+ DORIS_CHECK(metadata.row_groups[0].columns.size() > 2);
+ // Reuse the immutable chunks to isolate the scheduler decision: each
row-group reader owns an
+ // independent cursor, while the second footer entry deliberately cannot
prove an empty residual.
+ auto second_row_group = metadata.row_groups[0];
+ auto& root_residual = second_row_group.columns[2].meta_data.statistics;
+ DORIS_CHECK(root_residual.__isset.null_count);
+ root_residual.__set_null_count(second_row_group.num_rows - 1);
Review Comment:
[P2] Exercise real residual data in the fallback row group
This duplicates the first row group's Column Chunks and changes only footer
`null_count`; the underlying terminal residual page is still all null. As a
result, the test's rows, typed `n` values, and projection counters are
unchanged even if reader construction or deferred I/O mistakenly keeps using
the leaf-only request for the row group marked as full projection. Please use a
real second row group with a non-null terminal fallback value and assert the
value reconstructed from that branch, so the test fails when the
row-group-scoped physical shape is ignored.
##########
regression-test/suites/external_table_p0/paimon/test_paimon_catalog_variant.groovy:
##########
@@ -252,12 +254,52 @@ suite("test_paimon_catalog_variant",
"p0,external,doris,external_docker,external
select id,
cast(payload['name'] as string),
cast(payload['age'] as int),
- cast(payload['extra'] as string)
+ cast(payload['extra'] as string),
+ cast(payload['profile']['address']['city'] as string),
+ cast(payload['profile']['address']['zip'] as int)
from variant_shredded
where cast(payload['age'] as int) >= 20
order by id
"""
+ order_qt_native_shredded_deep_object_projection """
+ select id,
+ cast(payload['profile']['address']['zip'] as int),
+ cast(payload['profile']['address']['rank'] as int)
+ from variant_shredded
+ order by id
+ """
+
+ sql """set enable_profile = true"""
+ sql """set profile_level = 2"""
+ String deepProjectionToken =
+ "paimon_variant_deep_object_projection_" +
UUID.randomUUID().toString()
+ List<List<Object>> deepProjectionRows = sql """
+ select '${deepProjectionToken}', id,
+ cast(payload['profile']['address']['zip'] as int),
+ cast(payload['profile']['address']['rank'] as int)
+ from variant_shredded
+ order by id
+ """
+ assertEquals(2, deepProjectionRows.size())
+ String deepProjectionProfile = new
ProfileAction(context).getProfileBySql(
+ deepProjectionToken, ["VariantLeafProjections"], 30000L, 500L)
Review Comment:
[P2] Assert the actual row-group projection outcome
`VariantLeafProjections` is now incremented from the candidate count in
`ParquetReader::open()`, before any row-group residual statistics decide
whether the candidate is retained. This fixture also writes `tags`/`extra`
outside its shredding schema, so the root residual is populated and the current
finalizer can full-project every row group while this assertion still passes.
Poll `VariantLeafProjectionRowGroupColumns` (and ideally require
`VariantFullProjectionRowGroupColumns == 0`) against a fully shredded fixture
so an always-fallback regression cannot satisfy the test.
--
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]
