github-actions[bot] commented on code in PR #66206:
URL: https://github.com/apache/doris/pull/66206#discussion_r3679333166
##########
be/src/core/column/variant_v2/column_variant_v2.cpp:
##########
@@ -497,6 +554,9 @@ void ColumnVariantV2::for_each_subcolumn(ColumnCallback
callback) const {
}
void ColumnVariantV2::mutate_subcolumns() {
+ if (_shredded) {
+ ensure_encoded();
Review Comment:
[P1] Preserve leaf projections across output detachment
A retained leaf-only state reaches this method during every normal
table-reader handoff.
`finalize_chunk()` calls `IColumn::mutate()` on each mapped output; nullable
mutation recurses
into the nested `ColumnVariantV2`, and this `ensure_encoded()` then calls
`materialized_column()`, which intentionally throws while `_complete` is
false. A trivial
Variant mapping is just a `VSlotRef`, and even the last mapping's detach
helper mutates it, so
`SELECT v['n']` fails here before the upper element expression can consume
the validated leaf.
Please make ownership detachment preserve/share immutable shredded state and
defer canonical
encoding until an operation actually needs full logical values; add an
end-to-end
`TableReader` leaf-projection test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -1200,6 +1200,11 @@ private List<Split> doGetSplits(int numBackends) throws
UserException {
return doGetSystemTableSplits();
}
+ boolean projectsVariant = !isTableLevelCountStarPushdown()
+ && desc.getSlots().stream()
+ .anyMatch(slot ->
IcebergUtils.containsVariant(slot.getColumn().getType()));
Review Comment:
[P1] Gate smooth upgrades from the effective projected slot type
This checks the original Iceberg `Column`, not the scan slot type after
nested-column pruning.
`SlotTypeReplacer` changes the `SlotReference` data type but retains
`originalColumn`, and
`PlanTranslatorContext` consequently stores the full original column while
serializing the
pruned type in the descriptor. For `STRUCT<label: STRING, payload:
VARIANT>`, selecting only
`info.label` sends no Variant to BE but is still rejected whenever a
smooth-upgrade source is
present. Derive this capability check from `slot.getType()` (and
materialized semantic slots)
and add a non-Variant sibling projection case.
##########
be/src/core/column/variant_v2/column_variant_v2.cpp:
##########
@@ -724,6 +792,20 @@ void ColumnVariantV2::insert_range_from( //
NOLINT(readability-function-size)
return;
}
+ if (!_shredded && !_typed && empty() && _metadatas->empty() &&
source._shredded && start == 0 &&
+ length == source.size()) {
+ _shredded = source._shredded;
+ _check_invariants();
+ return;
+ }
+ if (_shredded) {
+ ensure_encoded();
+ }
+ if (source._shredded) {
+ insert_range_from(source._shredded->materialized_column(), start,
length);
Review Comment:
[P1] Keep subset slices of leaf projections shredded
Fixing the ownership-detach path does not make proper slices usable.
`IColumn::cut()` creates
an empty destination and calls `insert_range_from()`; for a proper subset
the full-source
sharing branch above does not apply, so this line asks an intentionally
incomplete projection
for its canonical column and throws. `ParquetScanScheduler` reaches this
after an all-filtered
prefix widens predicate probing and a later probe has more survivors than
`_batch_size`:
`materialize_pending_predicate_batch()` slices every retained
predicate/output column with
`column->cut(...)`. Please give the shredded state native range/index
selection (or otherwise
implement subset insertion without full materialization) and cover that
widened-batch path.
##########
be/src/format_v2/parquet/native_schema_desc.cpp:
##########
@@ -55,6 +56,180 @@ 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 &&
+ typed_value->parquet_schema.repetition_type !=
tparquet::FieldRepetitionType::OPTIONAL) {
+ return Status::Corruption("Parquet Variant {} typed_value must be
optional",
+ group_schema.name);
+ }
+
+ std::function<Status(const NativeFieldSchema&)> validate_typed_value;
+ std::function<Status(const NativeFieldSchema&)> validate_wrapper;
+ validate_wrapper = [&](const NativeFieldSchema& wrapper) -> Status {
+ if (!wrapper.parquet_schema.__isset.repetition_type ||
+ wrapper.parquet_schema.repetition_type !=
tparquet::FieldRepetitionType::REQUIRED) {
+ return Status::Corruption("Parquet Variant shredded wrapper {}
must be required",
+ wrapper.name);
+ }
+ const NativeFieldSchema* fallback = nullptr;
+ const NativeFieldSchema* typed = nullptr;
+ for (const auto& child : wrapper.children) {
+ if (child.name == "value") {
+ fallback = &child;
+ } else if (child.name == "typed_value") {
+ typed = &child;
+ } else {
+ return Status::Corruption("Parquet Variant wrapper {} has
unexpected child {}",
+ wrapper.name, child.name);
+ }
+ }
+ if (fallback == nullptr || typed == nullptr ||
!fallback->children.empty() ||
Review Comment:
[P1] Accept valid omitted children in shredded wrappers
Requiring both schema children rejects conforming Variant shredding layouts.
The
[Parquet
specification](https://parquet.apache.org/docs/file-format/types/variantshredding/)
permits an object-field wrapper to omit `typed_value` when that field is not
shredded as a
specific type, and an ARRAY element wrapper may omit `value` when fully
typed or omit
`typed_value` when untyped (with at least one representation present).
`append_wrapper()`
already handles a missing child, but these files fail during schema open
before decoding.
Please validate the context-specific allowed shapes, validate each present
child, and recurse
only when `typed_value` exists; add schema-open coverage for value-only
object fields and
typed-only/value-only ARRAY elements.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -627,8 +672,15 @@ Status FileScannerV2::_create_table_reader_for_format(
Status FileScannerV2::_prepare_table_reader_split(const TFileRangeDesc& range,
std::map<std::string, Field>
partition_values) {
+ const auto format_type = get_range_format_type(*_params, range);
+ const bool projects_variant =
+ !_local_state->is_count_star_pushdown() &&
+ std::ranges::any_of(_projected_columns, [](const
format::ColumnDefinition& column) {
+ return contains_variant_type(column.type);
+ });
+ RETURN_IF_ERROR(validate_iceberg_variant_file_format(range, format_type,
projects_variant));
Review Comment:
[P1] Gate only splits that physically read Variant data
This rejects from the logical projected type before `prepare_split()` can
apply
partition/runtime-filter pruning, while the earlier
`_build_projected_columns()` check rejects
before per-file schema mapping. That breaks valid schema evolution: the new
regression itself
writes an ORC file containing only `id`, then adds `v VARIANT`; the mapper
should synthesize
NULL for that old file, and a mixed table should decode Variant only from
its newer Parquet
file. Instead both queries fail even though no ORC Variant value exists.
Delay the capability
check until the split survives pruning and mapping proves a requested
Variant has a physical
source, and make the evolved-ORC/mixed-format cases return NULL plus the
Parquet value.
--
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]
