github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3682477689
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -1098,28 +1098,40 @@ private TableScan buildScan(Table table,
IcebergTableHandle handle, Optional<Con
TableScan scan = table.newScan();
// MVCC / time-travel pin: a tag/branch pins by REF (so a later commit
to the ref is honored, legacy
// parity), else by snapshot id (legacy createTableScan: useRef when
info.getRef()!=null else useSnapshot).
- if (handle.hasSnapshotPin()) {
+ if (handle.hasSnapshotPin() && supportsSnapshotSelection(handle)) {
if (handle.getRef() != null) {
scan = scan.useRef(handle.getRef());
} else {
scan = scan.useSnapshot(handle.getSnapshotId());
}
}
if (filter.isPresent()) {
- // Predicate conversion uses the table's CURRENT schema, matching
legacy createTableScan:589
- // (convertToIcebergExpr(conjunct, icebergTable.schema())) — NOT
the pinned schema. A predicate on a
- // column renamed since the pinned snapshot then resolves to no
field and drops to BE residual,
- // exactly like legacy; the common no-rename case is identical
(the pinned name == the current name),
- // and the unbound expression still binds against the pinned
snapshot's schema at plan time.
+ // Historical predicates must resolve names to the field ids of
the generation used for binding;
+ // using the current schema drops renamed predicates or can bind a
later reused name incorrectly.
+ Schema predicateSchema = handle.hasSnapshotPin() ?
pinnedSchema(table, handle) : table.schema();
List<Expression> predicates =
- new IcebergPredicateConverter(table.schema(),
resolveSessionZone(session)).convert(filter.get());
+ new IcebergPredicateConverter(predicateSchema,
resolveSessionZone(session)).convert(filter.get());
for (Expression predicate : predicates) {
scan = scan.filter(predicate);
}
}
return scan;
}
+ /** Whether this table type's scan can actually honor Iceberg's
snapshot/ref selection APIs. */
+ private static boolean supportsSnapshotSelection(IcebergTableHandle
handle) {
+ if (!handle.isSystemTable()) {
+ return true;
+ }
+ MetadataTableType type =
MetadataTableType.from(handle.getSysTableName());
+ // Static metadata tables enumerate metadata history independently of
the selected snapshot. Calling
+ // useSnapshot/useRef only rejects expired pins while leaving their
returned rows unfenced.
+ return type != MetadataTableType.HISTORY
+ && type != MetadataTableType.SNAPSHOTS
+ && type != MetadataTableType.REFS
+ && type != MetadataTableType.METADATA_LOG_ENTRIES;
Review Comment:
[P1] Exclude the `ALL_*` metadata tables from snapshot selection too
This guard still returns true for `ALL_DATA_FILES`, `ALL_DELETE_FILES`,
`ALL_FILES`, `ALL_MANIFESTS`, and `ALL_ENTRIES`. Those scans inherit Iceberg
1.10.1's
[`BaseAllMetadataTableScan`](https://github.com/apache/iceberg/blob/apache-iceberg-1.10.1/core/src/main/java/org/apache/iceberg/BaseAllMetadataTableScan.java),
whose `useSnapshot` and `useRef` both throw `UnsupportedOperationException`
because they enumerate files/manifests reachable from all tracked snapshots.
Since the system handle retains the base snapshot/ref pin, a query such as
`t$all_data_files FOR VERSION AS OF ...` reaches `buildScan()` and fails here
before planning. Please classify the `ALL_*` types as unsupported alongside the
four static tables and add a pinned `all_data_files` (plus ref if practical)
test. This is the plugin-path counterpart of the earlier legacy-path thread
that explicitly called out leaving `ALL_*` tables unfenced.
##########
regression-test/data/external_table_p0/iceberg/iceberg_query_tag_branch.out:
##########
@@ -6,7 +6,7 @@
1
-- !branch_3 --
-1 \N \N
+1
Review Comment:
[P1] Preserve every explicitly projected branch column
`qt_branch_3` still runs `select c1,c2,c3 ...`, but this new oracle row
contains only one field. The same truncation appears in `branch_18` and the
six-expression branch joins. The regression writer emits every JDBC row element
and does not strip trailing NULLs; the old oracle correctly represented this
row as `1\t\N\t\N`.
Iceberg's branch schema is the table's current schema
([`SnapshotUtil.schemaFor(table,
branch)`](https://github.com/apache/iceberg/blob/apache-iceberg-1.10.1/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java)
deliberately returns `table.schema()`), so `c2` and `c3` remain projected
columns even when the branch's old files supply NULL values. Please keep all
SELECT-list fields and regenerate the output. If this shortened row was
generated from the patched code, the underlying projection/result-metadata
regression needs to be fixed rather than recorded as expected behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java:
##########
@@ -417,7 +417,16 @@ && check(((MapType) originalType).getValueType(),
((MapType) targetType).getValu
return false;
}
for (int i = 0; i < targetFields.size(); i++) {
- if (originalFields.get(i).isNullable() !=
targetFields.get(i).isNullable()) {
+ // A nullable target can safely accept a required source, but
the inverse would
+ // allow a possible NULL into a required nested field.
+ if (originalFields.get(i).isNullable() &&
!targetFields.get(i).isNullable()) {
+ return false;
+ }
+ // A required source field is not sufficient: value conversion
itself can still
+ // produce NULL, so required targets only accept casts that
preserve non-nullness.
+ if (!targetFields.get(i).isNullable()
Review Comment:
[P2] Keep valid strict casts into required struct fields
This new guard is correct for non-strict casts, where a failed child
conversion becomes NULL, but it also runs when `isStrictMode` is true. In
strict mode a bad conversion throws instead of producing NULL, so a successful
required `STRING -> INT` child remains required.
For example:
```text
Cast(target STRUCT<metric:INT NOT NULL>)
named_struct('metric', '1') // source field is required STRING
```
The scalar strict cast is allowed and yields `1`, but
`Cast.castNullable(false, STRING, INT)` returns true here and rejects the
enclosing struct during analysis. Please gate the fallible-nullability
rejection to non-strict mode (or use a strict-aware predicate) and assert both
`CheckCast.check(requiredString, requiredInt, true)` and the existing
non-strict rejection.
--
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]
