github-actions[bot] commented on code in PR #69536:
URL: https://github.com/apache/doris/pull/69536#discussion_r4363903193
##########
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:
[P1] Preserve v3 temporal promotions when selecting the comparison type
Iceberg v3 permits DATE to unzoned TIMESTAMP/TIMESTAMP_NS promotion, but the
pinned 1.12.0 `TypeUtil.isPromotionAllowed` only recognizes INT/LONG,
FLOAT/DOUBLE and decimal precision
([spec](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/format/spec.md#schema-evolution),
[helper](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/api/src/main/java/org/apache/iceberg/types/TypeUtil.java)).
Iterating retained schemas oldest-first therefore keeps DATE, and
`withWidestTypes` replaces even a correctly selected TIMESTAMP lineage field
with DATE.
For an unpartitioned v3 table, retain a row with `k=DATE '2024-01-02'`,
promote k to TIMESTAMP, commit an equality delete for `2024-01-02 12:00:00`,
then drop k. Omit column metrics so the delete reaches the reader. V2
`_resolve_equality_delete_fields` chooses carrier DATE and
`_load_equality_delete_file` casts the noon tombstone to DATE: it incorrectly
deletes the original row, whose promoted value is midnight. The previous
history lookup retained TIMESTAMP. Handle the v3 temporal comparison domains
explicitly, avoid narrowing the lineage type, and cover this sequence. This
differs from the existing INT/LONG thread because the promoted schema is
present on the lineage.
##########
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:
[P2] Retain schema-only optionality when recovering a dropped key
This consumes a key from `missing` using its snapshot-lineage definition
before the metadata fallback can observe a compatible required-to-optional
change. Consider a v2 unpartitioned table with required `k INT` and no initial
default: append S1, make k optional without a snapshot, prepare a NULL
equality-delete `RowDelta`, drop k, append an id-only file as S2, then commit
the pending delete as S3. The SDK allows this pending commit and stamps the
current dropped schema
([SnapshotProducer](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/SnapshotProducer.java),
[BaseRowDelta](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/BaseRowDelta.java)).
Use files without column metrics so pruning does not exclude the delete.
S2/S3 omit k; recovery reaches S1 and restores REQUIRED, while the previous
metadata lookup selected the intervening OPTIONAL definition. BE then rejects
S2 with `Missing required field: k` instead of materializing NULL for the
equality comparison. Type widening does not address this unchanged-INT case.
Preserve the compatible schema-only optionality transition and add this
pending-delete/drop/append test; globally relaxing still-required fields would
hide real errors.
--
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]
