github-actions[bot] commented on code in PR #66941:
URL: https://github.com/apache/doris/pull/66941#discussion_r3827927105
##########
be/src/format_v2/parquet/reader/variant_column_reader.cpp:
##########
@@ -1431,58 +1668,89 @@ class ParquetVariantShreddedState final : public
VariantShreddedState {
const ParquetColumnSchema* wrapper_schema = nullptr;
ColumnPtr wrapper = struct_child(*typed_schema, typed, key,
&wrapper_schema);
if (!wrapper) {
- return path_miss();
+ return seek_residual();
+ }
+ ColumnPtr residual = struct_child(*wrapper_schema, wrapper,
"value", nullptr);
+ typed = struct_child(*wrapper_schema, wrapper, "typed_value",
&typed_schema);
+ if (!typed) {
+ // The wrapper stores this field unshredded. Its residual
holds the whole value, so
+ // the remainder of the path resolves inside those bytes.
+ add_level(std::move(residual), nullptr);
+ return seek_residual();
+ }
+ add_level(std::move(residual), typed);
+
+ if (position + 1 < path.size()) {
Review Comment:
[P1] Reject conflicting carriers on the nonterminal primitive branch
For a path such as `b.c`, this branch records both `b.typed_value` and
`b.value` and then calls `seek_residual()`. If both are non-null,
`resolve_variant_path_rows()` forces the deepest level when no leaf schema
exists and returns `c` from `b.value`; canonical reconstruction instead reaches
`append_typed_value()` and rejects every primitive `typed_value` plus residual
pair as corruption. The existing terminal-carrier thread does not cover this
nonterminal branch. Please validate the pair before seeking the residual and
add a direct-vs-canonical `b.c` test with both carriers present.
##########
be/src/format_v2/parquet/reader/variant_column_reader.cpp:
##########
@@ -1211,6 +1220,191 @@ ColumnPtr normalize_materialized_path(const
ColumnVariantV2& materialized,
return ColumnNullable::create(std::move(values), std::move(nulls));
}
+// Column-level access to one optional Parquet column. Caching the null map
beside the values lets
+// the row loops resolve a path without repeating the ColumnNullable cast for
every row.
+struct NullableColumnView {
+ const IColumn* values = nullptr;
+ const uint8_t* nulls = nullptr;
+
+ bool present() const { return values != nullptr; }
+ bool is_null(size_t row) const {
+ return values == nullptr || (nulls != nullptr && nulls[row] != 0);
+ }
+};
+
+NullableColumnView view_column(const ColumnPtr& column) {
+ if (!column) {
+ return {};
+ }
+ if (const auto* nullable = check_and_get_column<ColumnNullable>(*column)) {
+ return {.values = &nullable->get_nested_column(),
+ .nulls = nullable->get_null_map_data().data()};
+ }
+ return {.values = column.get(), .nulls = nullptr};
+}
+
+// One `value`/`typed_value` pair along a shredded object path. Level 0 is the
Variant root and
+// level i + 1 belongs to path[i], so the residual at level i encodes the
value of that path prefix
+// for every row whose typed value beside it is null.
+struct ShreddedPathLevel {
+ ColumnPtr residual;
+ ColumnPtr typed;
+ NullableColumnView residual_view;
+ NullableColumnView typed_view;
+};
+
+// A residual can only supply the requested path for rows whose typed value is
null. Shredding keeps
+// an object's residual keys disjoint from its shredded fields, so a present
typed value always owns
+// the path and the residual beside it holds unrelated keys.
+bool residual_supplies_any_row(const ShreddedPathLevel& level, size_t rows) {
+ if (!level.residual_view.present()) {
+ return false;
+ }
+ for (size_t row = 0; row < rows; ++row) {
+ if (level.typed_view.is_null(row) &&
!level.residual_view.is_null(row)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+// Seeks object paths inside encoded residual bytes. Keys resolve to a
dictionary field id once per
+// distinct dictionary instead of once per row, and the dictionary index is
built only when a path
+// still has keys to resolve - a residual that already is the requested value
needs no dictionary.
+class VariantResidualSeeker {
+public:
+ using MetadataIndexProvider = std::function<const
UnshreddedMetadataIndex&()>;
+
+ VariantResidualSeeker(const NullableColumnView& metadata, size_t
path_length,
+ MetadataIndexProvider provider)
+ : _metadata(metadata), _path_length(path_length),
_provider(std::move(provider)) {}
+
+ // Resolves path[path_offset..] inside `value`. The offset keeps the cache
keyed by absolute
+ // path position, because different rows can enter the residual at
different depths.
+ bool seek(size_t row, StringRef value, size_t path_offset,
+ std::span<const VariantShreddedPathSegment> path, VariantRef*
output) {
+ DORIS_CHECK(output != nullptr);
+ if (path.empty()) {
+ // The residual already is the requested value; only its
dictionary travels with it.
+ if (_metadata.is_null(row)) {
+ return false;
+ }
+ const StringRef bytes = _metadata.values->get_data_at(row);
+ *output = VariantRef {.metadata = {.data = bytes.data, .size =
bytes.size},
+ .value = value};
+ return true;
+ }
+
+ const UnshreddedMetadataIndex& index = _index == nullptr ?
_resolve_index() : *_index;
+ const uint32_t dictionary = index.row_dictionary_ids[row];
+ if (dictionary == UnshreddedMetadataIndex::NULL_ROW) {
+ return false;
+ }
+ VariantRef current {.metadata = index.dictionaries[dictionary], .value
= value};
+ for (size_t position = 0; position < path.size(); ++position) {
+ if (path[position].kind !=
VariantShreddedPathSegment::Kind::OBJECT_KEY) {
+ // Array segments never reach a residual: a shredded array
lives in its typed_value,
+ // and the caller keeps those paths on the reconstruction path.
+ return false;
+ }
+ if (current.basic_type() != VariantBasicType::OBJECT) {
+ return false;
+ }
+ int64_t& field_id = _field_ids[dictionary * _path_length +
path_offset + position];
+ bool layout_validated = false;
+ if (field_id == UNRESOLVED_FIELD_ID) {
+ // object_find() validates the object layout before consulting
the dictionary, so
+ // resolving a key has to validate it here too.
+ static_cast<void>(current.num_elements());
+ layout_validated = true;
+ field_id = current.metadata.find_key(path[position].key);
+ }
+ if (field_id < 0) {
+ // A cached dictionary miss must not hide a corrupt object in
a later row.
+ if (!layout_validated) {
+ static_cast<void>(current.num_elements());
+ }
+ return false;
+ }
+ if (!current.object_find_by_id(static_cast<uint32_t>(field_id),
¤t)) {
Review Comment:
[P1] Preserve residual corruption checks on successful lookup
`object_find_by_id()` validates only field IDs visited by its binary search
and the selected child. A residual object can therefore contain a valid
requested key plus an unrelated member with an out-of-range field ID or
malformed value, and this fast path returns the requested value; canonical
reconstruction iterates every residual member through `object_value_at()` and
rejects the same row. This is distinct from the metadata-miss thread because
metadata is valid and lookup succeeds. Please validate the complete residual
object before publishing a hit or miss (cached once per row if needed), with a
differential corrupt-sibling test.
##########
be/src/format_v2/parquet/reader/variant_column_reader.cpp:
##########
@@ -1431,58 +1668,89 @@ class ParquetVariantShreddedState final : public
VariantShreddedState {
const ParquetColumnSchema* wrapper_schema = nullptr;
ColumnPtr wrapper = struct_child(*typed_schema, typed, key,
&wrapper_schema);
if (!wrapper) {
- return path_miss();
+ return seek_residual();
+ }
+ ColumnPtr residual = struct_child(*wrapper_schema, wrapper,
"value", nullptr);
+ typed = struct_child(*wrapper_schema, wrapper, "typed_value",
&typed_schema);
+ if (!typed) {
+ // The wrapper stores this field unshredded. Its residual
holds the whole value, so
+ // the remainder of the path resolves inside those bytes.
+ add_level(std::move(residual), nullptr);
+ return seek_residual();
+ }
+ add_level(std::move(residual), typed);
+
+ if (position + 1 < path.size()) {
+ // An intermediate typed_value that is not an object cannot
carry the rest of the
+ // path, so those rows resolve from the residual of this very
wrapper - the object
+ // form of this field is what did not match its shredded type.
The level just added
+ // is that residual; dropping it would search the parent
instead, where shredding
+ // guarantees this key can never appear.
+ if (typed_schema->kind != ParquetColumnSchemaKind::STRUCT) {
+ return seek_residual();
+ }
+ continue;
}
- if (ColumnPtr residual = struct_child(*wrapper_schema, wrapper,
"value", nullptr);
- static_cast<bool>(residual) && has_present_value(residual)) {
- // A residual value can contribute data to the same logical
object. Reconstructing
- // is required in that case; returning only the typed leaf
would drop information.
-
update_counter(_profile.variant_direct_leaf_residual_fallbacks, 1);
+ if (typed_schema->kind != ParquetColumnSchemaKind::PRIMITIVE ||
+ check_and_get_column<ColumnNullable>(*typed) == nullptr) {
+
update_counter(_profile.variant_direct_leaf_unsupported_fallbacks, 1);
+ if (!_complete) {
+ // Binary element_at evaluates complex prefixes before the
validated leaf.
+ // Serialize only retained descendants so projected-out
fields stay hidden.
+ if (auto normalized = find_normalized_value(path);
normalized.has_value()) {
+ return VariantShreddedTypedValue {
+ .column = nullptr, .type = nullptr,
.normalized = *normalized};
+ }
+ }
return std::nullopt;
}
- typed = struct_child(*wrapper_schema, wrapper, "typed_value",
&typed_schema);
- if (!typed) {
- return path_miss();
- }
- if (position + 1 == path.size()) {
- if (typed_schema->kind != ParquetColumnSchemaKind::PRIMITIVE ||
- check_and_get_column<ColumnNullable>(*typed) == nullptr) {
-
update_counter(_profile.variant_direct_leaf_unsupported_fallbacks, 1);
+ const bool needs_row_merge =
+ std::ranges::any_of(levels, [rows](const
ShreddedPathLevel& level) {
+ return residual_supplies_any_row(level, rows);
+ });
+ if (needs_row_merge) {
+ // Decoding a residual needs the root dictionary. A state that
projected the
+ // metadata away cannot serve those rows, so it keeps the
complete fallback.
+ if (!metadata.present()) {
+
update_counter(_profile.variant_direct_leaf_residual_fallbacks, 1);
if (!_complete) {
- // Binary element_at evaluates complex prefixes before
the validated leaf.
- // Serialize only retained descendants so
projected-out fields stay hidden.
+ // A projected state cannot rebuild its root, so
serialize the retained
+ // descendants instead of demanding the complete
Variant.
if (auto normalized = find_normalized_value(path);
normalized.has_value()) {
return VariantShreddedTypedValue {
.column = nullptr, .type = nullptr,
.normalized = *normalized};
}
}
return std::nullopt;
}
- if (!supports_direct_typed_variant_state(*typed_schema)) {
- if (_complete) {
-
update_counter(_profile.variant_direct_leaf_unsupported_fallbacks, 1);
- return std::nullopt;
- }
- // A partial projection cannot reconstruct its root.
Normalize only the exact
- // requested leaf so Parquet annotations survive
heterogeneous file schemas.
- update_counter(_profile.variant_direct_leaf_rows,
- static_cast<int64_t>(typed->size()));
- return VariantShreddedTypedValue {
- .column = nullptr,
- .type = nullptr,
- .normalized =
normalize_projected_primitive_leaf(*typed_schema, typed)};
- }
- update_counter(_profile.variant_direct_leaf_rows,
- static_cast<int64_t>(typed->size()));
- return VariantShreddedTypedValue {.column = std::move(typed),
- .type =
remove_nullable(typed_schema->type),
- .normalized = nullptr};
+ update_counter(_profile.variant_direct_leaf_rows,
static_cast<int64_t>(rows));
+
update_counter(_profile.variant_direct_leaf_residual_merged_rows,
+ static_cast<int64_t>(rows));
+ return VariantShreddedTypedValue {.column = nullptr,
+ .type = nullptr,
+ .normalized =
resolve_variant_path_rows(
+ typed_schema,
typed.get(), metadata, rows,
+ levels, path,
metadata_index)};
}
- if (typed_schema->kind != ParquetColumnSchemaKind::STRUCT) {
- return path_miss();
+
+ update_counter(_profile.variant_direct_leaf_rows,
static_cast<int64_t>(rows));
Review Comment:
[P1] Validate metadata before publishing a successful leaf
This successful direct/normalized handoff never validates the root
dictionary. Newly projectable binary/UUID/temporal leaves can therefore return
a plausible value with null or malformed metadata, while canonical
materialization rejects null metadata and calls
`VariantMetadataRef::validate()` for every non-outer-null row. In row groups
whose residual statistics are all null, `prune_variant_residual_columns_impl()`
even removes metadata from I/O, so the residual-miss thread cannot fix this
path. Please validate metadata before every successful handoff (and retain a
validation read when pruning), with a malformed-metadata
direct/normalized-vs-canonical 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]
