github-actions[bot] commented on code in PR #66413:
URL: https://github.com/apache/doris/pull/66413#discussion_r3759993803
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -674,11 +1073,54 @@ bool check_native_statistics(const
tparquet::FileMetaData& metadata,
}
add_slot_zonemap(&ctx, slot_index, column_schema->type,
std::move(zone_map));
}
- const auto result =
VExprContext::evaluate_zonemap_filter(request.conjuncts, ctx);
+ const auto result = VExprContext::evaluate_zonemap_filter(conjuncts, ctx);
accumulate_zonemap_stats(ctx, pruning_stats);
return result == ZoneMapFilterResult::kNoMatch;
}
+bool check_shredded_variant_statistics(
+ const tparquet::FileMetaData& metadata, const tparquet::RowGroup&
row_group,
+ const std::vector<std::unique_ptr<ParquetColumnSchema>>& file_schema,
+ const format::FileScanRequest& request, const cctz::time_zone*
timezone) {
+ for (const auto& conjunct : metadata_pruning_conjuncts(request)) {
+ const auto predicate = extract_variant_shredded_predicate(conjunct);
+ if (!predicate.has_value()) {
+ continue;
+ }
+ const auto shredding = resolve_variant_shredding(file_schema, request,
*predicate);
+ if (!shredding.has_value() || shredding->typed_value->leaf_column_id <
0 ||
+ shredding->typed_value->leaf_column_id >=
static_cast<int>(row_group.columns.size()) ||
+ !fallback_is_all_null(row_group, *shredding->fallback_value) ||
Review Comment:
[P1] Require every ancestor fallback to be empty before pruning a nested
Variant path. `resolve_variant_shredding()` overwrites `wrapper` while
descending and retains only the final `value` leaf, so this proves
`v['a']['b']`'s `b.value` is all-null but says nothing about `a.value`.
Row-level extraction deliberately checks every wrapper and reconstructs when an
ancestor residual is populated, since that residual can also supply `b`; typed
`b` bounds can therefore exclude a row group that still matches after
reconstruction. Please retain all encountered fallback leaves and require each
to pass the all-null proof (also in the Page Index path), with a nested-path
regression where the ancestor fallback is populated.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorComputeVariantType.java:
##########
@@ -0,0 +1,33 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.connector.converter;
+
+import org.apache.doris.catalog.VariantType;
+import org.apache.doris.thrift.TTypeDesc;
+
+/** Execution-only Variant type used by native external connector readers. */
+public final class ConnectorComputeVariantType extends VariantType {
Review Comment:
[P1] Preserve this execution marker across the planner and connector round
trips. `SlotReference.fromColumn` converts this subclass to the ordinary
Nereids `VariantType`, and `PlanTranslatorContext.createSlotDesc` then installs
a plain catalog `VariantType`; as a result `projectsComputeVariant()` misses
the scan, the v12 backend fence is skipped, and with the default
`enable_variant_v2=false` the slot thrift no longer advertises the V2 column
the native reader produces. The reverse path has the same loss:
`ConnectorColumnConverter.toConnectorType` emits `VARIANT`, so Iceberg compares
the bound schema with its live `VARIANT_COMPUTE_V2` mapping and rejects
delete-only MERGE as false schema drift. Please make the marker round-trip
(recursively for complex types) or carry this execution capability separately,
and add translated scan-tuple plus real row-level-write tests with the global
V2 switch off.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java:
##########
@@ -366,6 +373,53 @@ private static boolean hasMeaningfulTypeParameters(String
typeName) {
|| "TIMESTAMPTZ".equals(typeName);
}
+ static void validateWriteSchema(List<ConnectorColumn> columns, boolean
writesDataFiles) {
+ if (!writesDataFiles) {
+ return;
+ }
+ if (columns.stream().anyMatch(column ->
containsVariant(column.getType()))) {
+ // Reject the whole data-file write: validating only selected
columns would let an
+ // unchanged Variant target flow through a writer that cannot
preserve its physical identity.
+ throw new DorisConnectorException(
+ "Iceberg VARIANT columns are read-only and cannot be
written");
+ }
+ }
+
+ static void validateWriteSchema(ConnectorWriteHandle handle) {
+ // The explicit INSERT column list can omit an unchanged Variant
column, but the data writer
+ // still binds the complete target schema and therefore must validate
that complete shape.
+ validateWriteSchema(handle.getBoundTargetColumns(),
handle.isWritesDataFiles());
+ }
+
+ private static void validateNestedPartitionWriteCompatibility(
+ ConnectorWriteHandle handle, IcebergWriteSchemaContext
schemaContext) {
+ WriteOperation operation = handle.getWriteOperation();
+ boolean writesDataFiles = operation != WriteOperation.DELETE
+ && ((operation != WriteOperation.UPDATE && operation !=
WriteOperation.MERGE)
+ || handle.isWritesDataFiles());
+ if (!writesDataFiles || schemaContext == null
Review Comment:
[P1] Apply the nested-partition version fence to compaction `REWRITE` as
well. The earlier rolling-upgrade issue is fixed for DML paths whose
`schemaContext` is resolved here, but `planWrite()` deliberately sets it to
null for `WriteOperation.REWRITE`, so this condition returns.
`buildRewriteSink()` later recreates the context from the transaction table and
calls the same `buildSink()` data writer; on a nested partition spec, a
version-11 BE can therefore still fail resolving the nested source ID. Please
run the compatibility check after the rewrite table/context is available (or
perform an equivalent check there), and add a version-11 nested-partition
REWRITE case.
##########
regression-test/suites/external_table_p0/iceberg/test_iceberg_varbinary.groovy:
##########
@@ -22,20 +22,26 @@ suite("test_iceberg_varbinary", "p0,external") {
logger.info("disable iceberg test.")
return
}
+ sql "SET ENABLE_VARIANT_V2=true"
Review Comment:
[P1] This setup statement aborts the suite before any Variant path is
exercised. `enable_variant_v2` is an FE `Config` field, not a registered
session/global variable, so plain `SET` reaches `VariableMgr.setVar()` and
raises `ERR_UNKNOWN_SYSTEM_VARIABLE` (the same invalid statement is added to
`test_iceberg_variant_read.groovy` and `test_paimon_catalog_variant.groovy`).
Please remove these assignments and test the execution carrier with its
intended default-off configuration; if a separate case truly needs to mutate
the FE config, use the supported temporary FE-config helper and restore it.
--
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]
