morningman commented on code in PR #69536:
URL: https://github.com/apache/doris/pull/69536#discussion_r4366909872
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2327,8 +2341,40 @@ private static List<Schema> metadataSchemaHistory(Table
table) {
return new ArrayList<>(table.schemas().values());
}
- private static void addHistoricalEqualityFields(
- List<NestedField> fields, Set<Integer> missingFieldIds, Schema
historicalSchema) {
+ /**
+ * Field IDs are never reused and a field's type only changes by
promotion, which is monotonic, so the widest
+ * type a field has in any retained schema covers every file written for
it.
+ */
+ private static Map<Integer, Type> widestPromotedTypes(Set<Integer>
fieldIds, List<Schema> schemas) {
+ Map<Integer, Type> widestTypes = new HashMap<>();
+ for (Schema schema : schemas) {
+ for (Integer fieldId : fieldIds) {
+ Type type = schema.findType(fieldId);
+ if (type == null) {
+ continue;
+ }
+ Type widest = widestTypes.get(fieldId);
+ if (widest == null || TypeUtil.isPromotionAllowed(widest,
type.asPrimitiveType())) {
Review Comment:
Confirmed, and fixed in `cbf21fb0585`. The widening added in 71f51edf754
relied on `TypeUtil.isPromotionAllowed`, which in Iceberg 1.12.0 (and on main)
knows only INT→LONG, FLOAT→DOUBLE and decimal precision. For a v3 key promoted
from DATE to TIMESTAMP, the first retained type therefore won and replaced the
TIMESTAMP definition the lineage had already found. FileScannerV2 then cast the
noon tombstone to DATE with an ordinary truncating cast and deleted the
midnight row. V1 compares in the delete file's own type and was not affected.
Each field of the selected definition now starts from its own type and moves
only along a recognized promotion: the SDK rules plus DATE→unzoned
TIMESTAMP/TIMESTAMP_NS. It can no longer narrow a key or switch it to an
unrelated type, and the result does not depend on the order of the retained
schemas. `equalityCarrierKeepsLineageTimestampOfPromotedDateKey` replays this
sequence, and `equalityCarrierWidensDateKeyPromotedWithoutASnapshot` covers a
schema-only DATE→TIMESTAMP_NS promotion followed by a pending delete and a
drop. Both publish the promotion with `TableMetadata.Builder.setCurrentSchema`,
as `promotedDateBoundsDoNotPruneNanosecondMatches` does, because
`SchemaUpdate.updateColumn` in 1.12.0 still rejects it; PyIceberg and
iceberg-go reject it too unless incompatible changes are allowed, while a REST
catalog's `add-schema` update only checks the new schema on its own.
Master before this PR and branch-4.2 do not widen, so neither has this
regression. branch-4.2 does share the schema-only variant, because its lineage
yields DATE; that case goes into the note for the 4.x maintainers.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2280,38 +2280,52 @@ private Set<Integer>
cachedApplicableEqualityDeleteFieldIds(Table table, TableSc
* delete manifests does not enumerate data files or byte-split tasks, and
retained state is bounded by the
* number of equality keys rather than the table's entire schema history.
*
- * <p>The metadata schema list is searched in reverse chronology up to the
selected schema (schema IDs are
- * identifiers, not a sequence), so the latest definition of each stable
field ID wins. Current fields remain
- * first, allowing a dropped/re-added name to resolve the projected field
by name while a live historical
- * equality key resolves by its stable field ID.</p>
+ * <p>Schema IDs are identifiers, not a sequence: Iceberg reuses an ID
when evolution returns to an earlier
+ * schema, so a schema's position in the metadata list does not tell when
it was last active, and the list may
+ * also hold schemas committed after a time-travel or branch target. The
scan snapshot's parent chain is
+ * therefore searched first, so the definition active on that lineage
wins; the whole metadata list, newest
+ * first, is the fallback for schema-only changes and expired ancestors. A
dropped key nevertheless takes the
+ * widest type it has in any retained schema: a delete committed after a
schema-only promotion holds values of
+ * the promoted type even when no snapshot records it. Current fields
remain first, allowing a dropped/re-added
+ * name to resolve the projected field by name while a live historical
equality key resolves by its stable
+ * field ID.</p>
*/
@VisibleForTesting
static List<NestedField> schemaForPotentialEqualityDeletes(
Table table, TableScan scan, Schema scanSchema) {
return schemaForPotentialEqualityDeletes(
- table, scanSchema, applicableEqualityDeleteFieldIds(table,
scan));
+ table, scan.snapshot(), scanSchema,
applicableEqualityDeleteFieldIds(table, scan));
}
- private static List<NestedField> schemaForPotentialEqualityDeletes(
- Table table, Schema scanSchema, Set<Integer> equalityFieldIds) {
- List<Schema> metadataSchemas = metadataSchemaHistory(table);
- int selectedSchemaIndex = -1;
- for (int index = 0; index < metadataSchemas.size(); index++) {
- if (metadataSchemas.get(index).schemaId() ==
scanSchema.schemaId()) {
- selectedSchemaIndex = index;
- }
- }
- int lastRelevantIndex = selectedSchemaIndex >= 0
- ? selectedSchemaIndex : metadataSchemas.size() - 1;
+ @VisibleForTesting
+ static List<NestedField> schemaForPotentialEqualityDeletes(
+ Table table, Snapshot scanSnapshot, Schema scanSchema,
Set<Integer> equalityFieldIds) {
Set<Integer> missing = new HashSet<>(equalityFieldIds);
missing.removeAll(TypeUtil.indexById(scanSchema.asStruct()).keySet());
if (missing.isEmpty()) {
return scanSchema.columns();
}
List<NestedField> fields = new ArrayList<>(scanSchema.columns());
- for (int index = lastRelevantIndex; index >= 0 && !missing.isEmpty();
index--) {
- addHistoricalEqualityFields(fields, missing,
metadataSchemas.get(index));
+ List<Schema> metadataSchemas = metadataSchemaHistory(table);
+ Map<Integer, Type> widestTypes = widestPromotedTypes(missing,
metadataSchemas);
+ // Snapshots on one lineage mostly share a schema, so each schema is
indexed at most once.
+ Set<Integer> lineageSchemaIds = new HashSet<>();
+ for (Snapshot snapshot = scanSnapshot; snapshot != null &&
!missing.isEmpty();
+ snapshot = snapshot.parentId() == null ? null :
table.snapshot(snapshot.parentId())) {
+ Integer schemaId = snapshot.schemaId();
+ if (schemaId == null || !lineageSchemaIds.add(schemaId)) {
+ continue;
+ }
+ Schema lineageSchema = table.schemas().get(schemaId);
+ if (lineageSchema == null) {
+ throw new IllegalStateException(
+ "Iceberg snapshot schema " + schemaId + " is absent
from table metadata");
+ }
+ addHistoricalEqualityFields(fields, missing, lineageSchema,
widestTypes);
Review Comment:
Confirmed, and fixed in `cbf21fb0585`. The pending delete and the id-only
append both commit with the post-drop schema, so the lineage walk reaches the
original REQUIRED definition and never sees the schema-only
`makeColumnOptional`. Both readers then reject the id-only file: FileScannerV2
with `Missing required field: k`, and V1 with `Required Iceberg field 'k' is
missing from the data file and has no initial default`. Master's lookup found
OPTIONAL and read NULL. Column metrics do not prevent this: Iceberg's own
`DeleteFileIndex` resolves k from the highest schema ID, where it is optional,
and attaches the NULL tombstone because the new file has no stats for k.
A dropped key is now optional when any retained schema has it optional.
Required→optional is the compatible direction (the reverse needs
`allowIncompatibleChanges`), and reading NULL for the missing key is safe
either way. A key that is required in every retained schema stays required, so
the missing-required check does not change for it.
`equalityCarrierKeepsSchemaOnlyOptionalityOfDroppedKey` replays the
pending-delete/drop/append sequence, and
`equalityCarrierKeepsAlwaysRequiredDroppedKeyRequired` checks that an
always-required dropped key stays required.
branch-4.2 also resolves keys lineage-first in
`IcebergScanNode.getSchemaFieldsForScan`, and its readers reject the file the
same way, so it has the same defect; it goes into the note for the 4.x
maintainers.
--
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]
