github-actions[bot] commented on code in PR #66413:
URL: https://github.com/apache/doris/pull/66413#discussion_r3746400777
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonTypeMapping.java:
##########
@@ -97,6 +97,9 @@ public static ConnectorType toConnectorType(DataType
dataType, Options options)
return toTimestampType(dataType);
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
return toTimestampTzType(dataType, options);
+ case VARIANT:
+ // Preserve the execution-only carrier shared by JNI and
native Paimon readers.
+ return ConnectorType.of("VARIANT_COMPUTE_V2");
Review Comment:
[P1] Add the Paimon JNI transfer before exposing this type
This maps every Paimon Variant column to `TYPE_VARIANT` even when its split
is routed through JNI, including the new test's `force_jni_scanner=true` arm.
Both Paimon JNI readers publish that type through
`JniDataBridge::get_jni_type_with_different_string()`, which has no
`TYPE_VARIANT` case and sends `unsupported`; Java `ColumnType` and
`PaimonColumnValue` likewise have no Variant representation, so `VectorColumn`
throws `Unknown type value: UNSUPPORTED` on the first row. Please either
implement the JNI carrier/decoding end to end or keep Variant scans off this
path until it exists.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java:
##########
@@ -164,6 +165,7 @@ public IcebergWritePlanProvider(Map<String, String>
properties,
@Override
public ConnectorSinkPlan planWrite(ConnectorSession session,
ConnectorWriteHandle handle) {
+ validateWriteSchema(handle.getColumns(), handle.isWritesDataFiles());
Review Comment:
[P1] Validate the complete target schema for the Variant write fence
`getColumns()` is only the explicit write list, so `INSERT INTO
variant_write_guard (id) ...` reaches this call with `[id]` and bypasses the
guard even though the bound table still contains `payload VARIANT`; that is the
exact new regression at `test_iceberg_variant_read.groovy:1007`. The handle
already carries the full schema in `getBoundTargetColumns()`, so use that for
data-file-producing writes while retaining the `isWritesDataFiles()` exception
for delete-only MERGE.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SlotTypeReplacer.java:
##########
@@ -669,6 +670,10 @@ private void replaceAccessPathToFieldId(List<String>
originPath, int index, Data
break;
}
}
+ } else if (type instanceof VariantType) {
+ // Variant object keys are data, not Iceberg schema field IDs.
Replacing them with
+ // the root ID destroys the physical shredding path before it
reaches the scanner.
+ return;
Review Comment:
[P1] Keep Variant selectors out of the Iceberg field-ID set
Returning here preserves numeric Variant selectors as raw strings (the new
unit test records `v['arr'][0]` as `"0"`), but
`PluginDrivenScanNode.withProjectedFieldIds()` interprets every digit-only path
component as an Iceberg field ID. It therefore attaches `0` (or an object key
such as `"1"`) to the Variant column handle, and
`IcebergScanPlanProvider.projectedFieldIds()` rejects it because it is not that
column's schema descendant. Please preserve the segment kind or stop collecting
schema IDs once the path crosses a Variant boundary.
##########
be/benchmark/parquet/AGENTS.md:
##########
@@ -340,7 +350,7 @@ be simulated by silently changing the local reader
benchmark.
## Current validation record
-The current expected registration counts are 228 decoder, 92 kernel, 25
selection, 167 reader, and
+The current expected registration counts are 228 decoder, 92 kernel, 25
selection, 169 reader, and
Review Comment:
[P2] Update the kernel count in this validation record
The same changed document now reports 292 `ParquetKernel` cases at lines 54
and 149 after adding the 200 nullable-selection cases, but this record still
says 92. Anyone following the validation instructions therefore gets two
incompatible expected totals for the same binary; please update this count to
292 as well.
##########
be/src/exprs/vectorized_fn_call.cpp:
##########
@@ -671,11 +671,24 @@ bool VectorizedFnCall::is_deterministic() const {
}
bool VectorizedFnCall::is_safe_to_execute_on_selected_rows() const {
- static const std::set<std::string> TOTAL_PREDICATE_FUNCTIONS = {
- "eq", "ne", "lt", "le", "gt", "ge", "in", "not_in",
"is_null_pred", "is_not_null_pred"};
+ static const std::set<std::string> TOTAL_PREDICATE_FUNCTIONS = {"eq",
+
"eq_for_null",
+ "ne",
+ "lt",
+ "le",
+ "gt",
+ "ge",
+ "in",
+ "not_in",
+
"is_null_pred",
+
"is_not_null_pred",
+
"element_at",
Review Comment:
[P1] Fix the minimum array index before marking element_at total
A valid BIGINT selector of `INT64_MIN` reaches the negative-index check in
all three array implementations, where `-index` overflows signed `Int64` before
the intended out-of-range-to-NULL branch. Adding `element_at` here then makes
that undefined behavior execute or disappear depending on whether an earlier
staged predicate keeps the row, so it is not yet total/error-preserving. Please
make the negative bound check overflow-safe (or keep it out of this whitelist)
and cover `INT64_MIN` in the number, string, and common array paths.
--
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]
