github-actions[bot] commented on code in PR #66007:
URL: https://github.com/apache/doris/pull/66007#discussion_r3664803167
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hudi/HudiUtils.java:
##########
@@ -268,6 +269,25 @@ public static HudiMvccSnapshot
getHudiMvccSnapshot(Optional<TableSnapshot> table
return new
HudiMvccSnapshot(HudiUtils.getPartitionValues(tableSnapshot, hmsTable),
timestamp);
}
+ /**
+ * Resolve the instant used for split and schema planning.
+ */
+ public static Optional<String> resolveQueryInstant(Optional<MvccSnapshot>
relationSnapshot,
+ Optional<TableSnapshot> tableSnapshot, HoodieTimeline timeline) {
+ if
(relationSnapshot.filter(HudiMvccSnapshot.class::isInstance).isPresent()) {
+ long timestamp = ((HudiMvccSnapshot)
relationSnapshot.get()).getTimestamp();
+ // The pinned timestamp is authoritative even if the active
timeline advances later.
Review Comment:
[P1] Capture Hudi partitions at the same instant
This now makes the snapshot's timestamp authoritative for schema and split
planning, but `getHudiMvccSnapshot()` builds that object with two independent
latest lookups: `getLastTimeStamp()` can observe cached meta-client A/S0, then
a concurrent table-cache invalidation/refresh can make
`getPartitionValues(Optional.empty(), ...)` obtain client B/S1 and cache S1's
partitions. The relation consequently prunes with S1 partitions while
`queryInstant` enumerates files before/on S0; a partition removed or added by
S1 can silently omit or mis-prune S0 data. Please capture the completed instant
once and load the timestamp-keyed partition view for that exact instant, with a
barrier test that swaps the cached generation between the two current phases.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMvccSnapshot.java:
##########
@@ -29,4 +29,15 @@ public IcebergMvccSnapshot(IcebergSnapshotCacheValue
snapshotCacheValue) {
public IcebergSnapshotCacheValue getSnapshotCacheValue() {
return snapshotCacheValue;
}
+
+ @Override
+ public boolean isSameSnapshot(MvccSnapshot other) {
+ if (!(other instanceof IcebergMvccSnapshot)) {
+ return false;
+ }
+ IcebergSnapshot left = snapshotCacheValue.getSnapshot();
+ IcebergSnapshot right = ((IcebergMvccSnapshot)
other).snapshotCacheValue.getSnapshot();
+ // A branch can retain its data snapshot while adopting a newer
current schema.
+ return left.getSnapshotId() == right.getSnapshotId() &&
left.getSchemaId() == right.getSchemaId();
Review Comment:
[P1] Include frozen Iceberg semantic metadata in scan equality
Snapshot and schema IDs do not uniquely identify the relation semantics this
patch now freezes. `default.name.mapping` can change in a property-only
metadata commit without advancing either ID, and `IcebergScanNode` deliberately
reads that relation-local mapping because it changes how ID-less legacy file
columns resolve. Two ref/time-travel relations bound around that refresh
compare equal here, so `PullUpJoinFromUnionAll` may hoist one scan and apply
its aliases to both arms, changing values to or from NULL. Please include the
frozen mapping (or a full immutable metadata generation identity) in equality
and add a rewrite test with equal snapshot/schema IDs but different mappings.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -1408,6 +1432,24 @@ public IcebergTableQueryInfo getSpecifiedSnapshot()
throws UserException {
TableSnapshot tableSnapshot = getQueryTableSnapshot();
TableScanParams scanParams = getScanParams();
Optional<TableScanParams> params = Optional.ofNullable(scanParams);
+ Optional<MvccSnapshot> snapshot = getPinnedRelationSnapshot();
+ if (snapshot.isPresent() && snapshot.get() instanceof
IcebergMvccSnapshot) {
+ if (source.getTargetTable() instanceof IcebergSysExternalTable
+ && !((IcebergSysExternalTable)
source.getTargetTable()).supportsSnapshotSelection()) {
+ return null;
+ }
+ IcebergSnapshot pinnedSnapshot =
+ ((IcebergMvccSnapshot)
snapshot.get()).getSnapshotCacheValue().getSnapshot();
+ // Empty Iceberg tables use -1 as a cache sentinel; passing it to
useSnapshot would
+ // turn a valid empty scan (including metadata tables) into an
unknown-snapshot error.
+ if (pinnedSnapshot.getSnapshotId() < 0) {
Review Comment:
[P1] Preserve the bound empty state through scan planning
Returning `null` here turns the bound empty generation into “use whatever
snapshot the scan table currently has.” `doInitialize()` still loads that Table
independently through the refreshable source, so an empty S0 relation followed
by the first commit/cache refresh before physical init calls `newScan()` on S1
and returns rows that did not exist when the relation was bound. The added test
holds one unchanged empty mock and only proves that `useSnapshot(-1)` is
skipped. Please keep empty as an executable scan state (for example,
short-circuit normal data planning) or use the frozen Table generation, and add
an empty-bind/first-commit barrier test.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1486,8 +1505,8 @@ class TableReader {
_file_child_ordinal_for_mapping(mapping, *child_mapping,
file_ordered_children);
DORIS_CHECK(file_child_idx < file_struct->get_columns().size());
ColumnPtr child_column =
file_struct->get_column_ptr(file_child_idx);
-
RETURN_IF_ERROR(_materialize_present_child_mapping_column(*child_mapping,
child_column,
- rows,
&child_column));
+ RETURN_IF_ERROR(_materialize_present_child_mapping_column(
+ *child_mapping, child_column, rows, &child_column,
parent_null_map));
Review Comment:
[P1] Reject source parent NULLs for a required target struct
This passes the file parent's null map to required-child validation even
when `mapping.table_type` makes the target struct itself non-nullable. The new
waiver therefore accepts the child's NULL placeholder on a source row where the
optional parent is NULL, but the result path below preserves that parent map
only for a nullable target; otherwise it returns a bare `ColumnStruct`, turning
the row into a non-null struct containing the child's nested default value.
That silently satisfies neither source nor target semantics. Please allow this
mask only when the target parent is nullable (and reject any source parent NULL
otherwise), with optional-file to required-table tests for both a NULL row and
a present row.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java:
##########
@@ -414,7 +414,9 @@ && 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()) {
Review Comment:
[P1] Reject fallible casts into required struct fields
This accepts any declared non-null source field for a required target, but
declared nullability is not enough for a fallible conversion. For example,
`STRUCT('bad')` is now inferred with a required child, so a non-strict cast to
`STRUCT<metric:INT NOT NULL>` passes this check. During literal folding,
`checkedCastWithStrictChecking()` converts the invalid string to `NullLiteral`,
while `StructLiteral` still labels the result with the required target type; BE
then tries to materialize that NULL in a non-nullable child column. Please
reject required-target children when `Cast.castNullable(false, sourceType,
targetType)` is true, and cover a failing non-strict INSERT alongside the
successful widening case.
--
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]
