github-actions[bot] commented on code in PR #66206:
URL: https://github.com/apache/doris/pull/66206#discussion_r3673818619
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -1784,15 +1784,97 @@ static bool table_root_is_map(const ColumnMapping&
mapping) {
return remove_nullable(mapping.table_type)->get_primitive_type() ==
TYPE_MAP;
}
+static const ColumnDefinition* find_file_child_by_name(
+ const std::vector<ColumnDefinition>& children, std::string_view name) {
+ const auto child_it = std::ranges::find_if(
+ children, [name](const ColumnDefinition& child) { return
child.name == name; });
+ return child_it == children.end() ? nullptr : &*child_it;
+}
+
+static bool build_variant_leaf_path_projection(const ColumnMapping& mapping,
+ const std::vector<std::string>&
path,
+ LocalColumnIndex*
root_projection) {
+ DORIS_CHECK(root_projection != nullptr);
+ if (path.empty() || !mapping.file_local_id.has_value()) {
+ return false;
+ }
+ *root_projection = LocalColumnIndex::partial_local(*mapping.file_local_id);
+ const auto* root_typed =
find_file_child_by_name(mapping.original_file_children, "typed_value");
+ if (root_typed == nullptr || root_typed->children.empty()) {
Review Comment:
[P1] Restrict leaf projection traversal to shredded objects
A conforming shredded array stores its typed payload as a LIST whose
physical child is named `element`. A query such as `v['element']` is an
object-key lookup on that array and should be missing/NULL, but this code can
match the LIST child as though it were an object field and retain an incomplete
projection when the residuals are all null. Extraction then rejects the
non-STRUCT root in `find_typed_value()`, and the fallback cannot materialize an
incomplete state, so the query fails with `A projected Parquet Variant can only
serve its validated shredded leaves`. Please require the root and each
intermediate `typed_value` to be STRUCT/object nodes before matching key names,
and cover a shredded array with an `element` key lookup.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -713,7 +713,11 @@ public static Type
icebergTypeToDorisType(org.apache.iceberg.types.Type type, bo
.collect(Collectors.toCollection(ArrayList::new));
return new StructType(nestedTypes);
case VARIANT:
- return Type.UNSUPPORTED;
+ // Iceberg Variant uses the Parquet Variant encoding directly.
Mark it compute-only
+ // so BE scanners materialize ColumnVariantV2 without changing
persisted Doris
+ // table metadata semantics.
+ return new org.apache.doris.catalog.VariantType(
Review Comment:
[P1] Gate Iceberg DML until the writer supports VARIANT
Separately from the read-side upgrade gate, returning a normal Doris type
here also lets `BindSink.bindIcebergTableSink()` plan INSERT/rewrite operations
for tables containing VARIANT. `IcebergTableSink` always serializes the
complete Iceberg schema, and `VIcebergTableWriter::open()` feeds it to the BE
Iceberg `SchemaParser`; its `TypeID`/`Types::from_primitive_string()` has no
`variant` case and throws `Cannot parse type string to primitive: variant`.
Consequently any Doris INSERT into such a table fails only after reaching the
sink, even when the VARIANT column was omitted, while the new tests avoid the
path by writing through Spark. If this feature is read-only, reject Iceberg
DML/rewrite during FE analysis for root or nested VARIANT; otherwise the writer
stack and a Doris INSERT test need full support.
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -614,12 +927,17 @@ void collect_filtered_leaf_ids(const ParquetColumnSchema&
column_schema,
return;
}
for (const auto& child_schema : column_schema.children) {
- if (!format::is_child_projected(projection, child_schema->local_id)) {
+ if (column_schema.kind != ParquetColumnSchemaKind::VARIANT &&
Review Comment:
[P2] Account only for the retained Variant projection
Once `finalize_variant_leaf_projections()` retains a leaf-only Variant
projection, reconstruction no longer reads every physical sibling. This special
case still traverses the entire Variant and
`native_requested_compressed_bytes()` consequently adds metadata, residual, and
unrelated typed chunks to `FilteredBytes`, even though prefetch/readers request
only the projected leaf. Please follow the actual projection here (using the
full sibling set only for a full Variant projection) so the pruning profile
reports bytes that would really have been read.
##########
be/src/format_v2/parquet/native_schema_desc.cpp:
##########
@@ -55,6 +55,84 @@ static bool is_map_node(const tparquet::SchemaElement&
schema) {
(schema.__isset.logicalType && schema.logicalType.__isset.MAP);
}
+static bool is_variant_node(const tparquet::SchemaElement& schema) {
+ return schema.__isset.logicalType && schema.logicalType.__isset.VARIANT;
+}
+
+class ScopedBoolOverride {
+public:
+ ScopedBoolOverride(bool& target, bool value) : _target(target),
_original(target) {
+ _target = value;
+ }
+ ~ScopedBoolOverride() { _target = _original; }
+
+private:
+ bool& _target;
+ bool _original;
+};
+
+static Status validate_variant_layout(const tparquet::SchemaElement&
group_schema,
+ const NativeFieldSchema& group_field) {
+ const auto& annotation = group_schema.logicalType.VARIANT;
+ if (annotation.__isset.specification_version &&
annotation.specification_version != 1) {
+ return Status::NotSupported("Parquet Variant specification version {}
is not supported",
+ annotation.specification_version);
+ }
+ if (group_field.children.size() < 2 || group_field.children.size() > 3) {
+ return Status::Corruption(
+ "Parquet Variant {} must contain metadata, value, and optional
typed_value",
+ group_schema.name);
+ }
+
+ const NativeFieldSchema* metadata = nullptr;
+ const NativeFieldSchema* value = nullptr;
+ const NativeFieldSchema* typed_value = nullptr;
+ for (const auto& child : group_field.children) {
+ const NativeFieldSchema** target = nullptr;
+ if (child.name == "metadata") {
+ target = &metadata;
+ } else if (child.name == "value") {
+ target = &value;
+ } else if (child.name == "typed_value") {
+ target = &typed_value;
+ } else {
+ return Status::Corruption("Parquet Variant {} has unexpected child
{}",
+ group_schema.name, child.name);
+ }
+ if (*target != nullptr) {
+ return Status::Corruption("Parquet Variant {} has duplicate child
{}",
+ group_schema.name, child.name);
+ }
+ *target = &child;
+ }
+ if (metadata == nullptr || value == nullptr) {
+ return Status::Corruption("Parquet Variant {} requires metadata and
value children",
+ group_schema.name);
+ }
+ if (!metadata->children.empty() || metadata->physical_type !=
tparquet::Type::BYTE_ARRAY ||
+ metadata->parquet_schema.repetition_type !=
tparquet::FieldRepetitionType::REQUIRED) {
+ return Status::Corruption("Parquet Variant {} metadata must be a
required BYTE_ARRAY",
+ group_schema.name);
+ }
+ const auto expected_value_repetition = typed_value == nullptr
+ ?
tparquet::FieldRepetitionType::REQUIRED
+ :
tparquet::FieldRepetitionType::OPTIONAL;
+ // SQL nullability belongs to the outer Variant group. Only shredding
makes value optional,
+ // because typed_value may carry all or part of the logical value instead.
+ if (!value->children.empty() || value->physical_type !=
tparquet::Type::BYTE_ARRAY ||
+ value->parquet_schema.repetition_type != expected_value_repetition) {
+ return Status::Corruption("Parquet Variant {} value must be a {}
BYTE_ARRAY",
+ group_schema.name,
+ typed_value == nullptr ? "required" :
"optional");
+ }
+ if (typed_value != nullptr &&
Review Comment:
[P2] Validate the complete shredded schema at open
This validates only the root wrapper and then lets the ordinary Parquet type
mapper accept any optional `typed_value` subtree. The Variant shredding
specification also fixes the nested shape and scalar matrix: named object-field
and LIST-element wrappers are required, and types such as unsigned INTEGER,
adjusted TIME, or TIME(MILLIS) are not valid shredded scalars. The current gap
makes malformed input projection-dependent—for example, root reconstruction
rejects an unsigned typed leaf in `integer_width()`, while the direct leaf path
returns its widened Doris column without reaching that check. Please
recursively validate every wrapper and typed leaf here so nonconforming
shredding fails once at schema open, and add negative schema tests that
exercise both root and narrow projections.
##########
be/src/exec/scan/access_path_parser.cpp:
##########
@@ -470,6 +470,44 @@ Status
AccessPathParser::build_nested_children(format::ColumnDefinition* column,
if (is_scanner_materialized_virtual_column(column->name)) {
return Status::OK();
}
+ if (remove_nullable(column->type)->get_primitive_type() == TYPE_VARIANT) {
+ column->variant_access_paths.clear();
+ for (const auto& access_path : access_paths) {
+ if (access_path.type != TAccessPathType::DATA ||
+ !access_path.__isset.data_access_path) {
+ return Status::NotSupported(
+ "AccessPathParser only supports DATA access paths for
Variant slot {}",
+ column->name);
+ }
+ const auto& path = access_path.data_access_path.path;
+ if (path.empty()) {
+ // Match the generic access-path tree: an empty DATA path
denotes the whole slot
+ // and dominates every narrower Variant path in the same
request.
+ column->variant_access_paths.clear();
+ return Status::OK();
+ }
+ int32_t top_level_id = -1;
+ if (to_lower(path.front()) != to_lower(column->name) &&
+ (!parse_non_negative_int(path.front(), &top_level_id) ||
+ !column->has_identifier_field_id() ||
+ top_level_id != column->get_identifier_field_id())) {
+ return Status::NotSupported(
+ "AccessPathParser access path {} does not match
Variant slot {}",
+ access_path_to_string(path), column->name);
+ }
+ if (path.size() == 1) {
+ // A whole-root access covers every subpath and must disable
physical leaf pruning.
+ column->variant_access_paths.clear();
+ return Status::OK();
+ }
+ column->variant_access_paths.emplace_back(path.begin() + 1,
path.end());
Review Comment:
[P1] Do not leaf-project lossy Variant access paths
By the time these strings reach this branch, FE has already lost several
distinctions that are semantic for Variant access: `v[1]` and `v['1']` both
produce `"1"`; the `TreeMultimap` comparator dot-joins components, so
`v['a.b']` and `v['a']['b']` compare equal; and a nested literal key `"NULL"`
is stripped as though it were the null-metadata sentinel. The mapper
nevertheless treats each surviving string sequence as an exact object path and
may retain an incomplete leaf projection. On conforming fully shredded objects,
evaluation preserves the original expression path, cannot find the mismatched
or omitted typed leaf, and then fails because the incomplete Variant cannot be
materialized. Please preserve segment kind, component boundaries, and sentinel
provenance end to end—or disable leaf projection unless the path is provably
lossless—and add differential tests for all three collisions.
--
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]
