morningman commented on code in PR #69536:
URL: https://github.com/apache/doris/pull/69536#discussion_r4367350256
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2327,8 +2341,55 @@ 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) {
+ /**
+ * A writer commits with the schema that is current at commit, so a key
dropped before its delete commits loses
+ * the schema-only changes made before the drop from every snapshot. Field
IDs are never reused and a field
+ * only relaxes compatibly, by promotion or from required to optional, so
each field of the selected definition
+ * takes the widest promotion of its type and the optionality it has in
any retained schema; neither depends on
+ * the order of the schemas. A field required in every retained schema
stays required.
+ */
+ private static List<NestedField> withRetainedRelaxations(List<NestedField>
fields, List<Schema> schemas) {
+ List<NestedField> relaxed = new ArrayList<>(fields.size());
+ for (NestedField field : fields) {
+ Type type = field.type();
+ boolean optional = field.isOptional();
+ for (Schema schema : schemas) {
+ NestedField retained = schema.findField(field.fieldId());
+ if (retained == null) {
+ continue;
+ }
+ optional |= retained.isOptional();
+ if (type.isPrimitiveType() && retained.type().isPrimitiveType()
+ && isWideningPromotion(type.asPrimitiveType(),
retained.type().asPrimitiveType())) {
+ type = retained.type();
+ }
+ }
+ if (type.isStructType()) {
+ type =
Types.StructType.of(withRetainedRelaxations(type.asStructType().fields(),
schemas));
+ }
+ relaxed.add(type.equals(field.type()) && optional ==
field.isOptional()
+ ? field :
Types.NestedField.from(field).ofType(type).isOptional(optional).build());
Review Comment:
Confirmed, and fixed in `36bac832997`. `NestedField`'s constructor casts
both defaults to the field type (`castDefault` → `Literal.to`), and in Iceberg
1.12.0 `DateLiteral.to` returns null for anything but DATE. Rebuilding a
dropped DATE key whose lineage definition carries an initial or write default
as TIMESTAMP/TIMESTAMP_NS therefore threw `Cannot cast default value to
timestamp` and failed planning for every scan the delete applies to. Master
took the already-typed TIMESTAMP definition and never rebuilt it, and
branch-4.2 does not widen, so neither fails this way. The SDK promotions
(INT→LONG, FLOAT→DOUBLE, decimal precision) convert their literals already.
The relaxation now maps a DATE default to midnight of that day in the
promoted type and then rebuilds the field. A date outside the TIMESTAMP_NS
range still fails, as the spec requires for this promotion.
`equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestamp` and
`equalityCarrierConvertsDefaultsOfDateKeyPromotedToTimestampNs` replay your
sequence with both defaults set, for TIMESTAMP and TIMESTAMP_NS, and check the
converted default in the scan dictionary too.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2327,8 +2341,55 @@ 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) {
+ /**
+ * A writer commits with the schema that is current at commit, so a key
dropped before its delete commits loses
+ * the schema-only changes made before the drop from every snapshot. Field
IDs are never reused and a field
+ * only relaxes compatibly, by promotion or from required to optional, so
each field of the selected definition
+ * takes the widest promotion of its type and the optionality it has in
any retained schema; neither depends on
+ * the order of the schemas. A field required in every retained schema
stays required.
+ */
+ private static List<NestedField> withRetainedRelaxations(List<NestedField>
fields, List<Schema> schemas) {
+ List<NestedField> relaxed = new ArrayList<>(fields.size());
+ for (NestedField field : fields) {
+ Type type = field.type();
+ boolean optional = field.isOptional();
+ for (Schema schema : schemas) {
+ NestedField retained = schema.findField(field.fieldId());
+ if (retained == null) {
+ continue;
+ }
+ optional |= retained.isOptional();
+ if (type.isPrimitiveType() && retained.type().isPrimitiveType()
+ && isWideningPromotion(type.asPrimitiveType(),
retained.type().asPrimitiveType())) {
+ type = retained.type();
Review Comment:
The mechanism is right. If a dropped key is restored with
`set-current-schema` and then promoted to TIMESTAMP_NS, a read pinned before
the promotion now compares in TIMESTAMP_NS. FileScannerV2's strict DATE→nanos
`EvolutionCast` then rejects any date outside 1677-09-21..2262-04-11 with
`TIMESTAMP_NS overflow when casting row …`. The query fails closed; no row is
dropped or kept by mistake.
I'm keeping the widening, because the FE cannot tell whether a retained
schema was added before or after the scan target. Schemas carry no timestamp,
schema IDs are reused (the defect this PR fixes), and the list position only
records when a schema was first added. If we stopped widening along
DATE→TIMESTAMP_NS for a schema that might be later than the target, the case
covered by `equalityCarrierWidensDateKeyPromotedWithoutASnapshot` would come
back: a delete prepared after a schema-only promotion holds nanosecond
tombstones, and a DATE carrier makes FileScannerV2 truncate them to their date
and delete rows silently. A fail-closed error is the safer of the two outcomes.
Your history also needs a rollback of the drop through `set-current-schema`, a
DATE→TIMESTAMP_NS promotion that the 1.12.0 schema-update API rejects, and
dates outside the nanosecond range. After that promotion the table's current
read fails on the same values anyway, as the spec requires.
The complete fix belongs in FileScannerV2, which compares every delete file
in the one carrier type. Choosing the comparison type per delete file, from its
own key type and the data file's (V1 already uses the delete file's type),
would handle this case. It would also handle the related one where
`expireSnapshots().cleanExpiredMetadata(true)` removes a schema-only promotion
and leaves the carrier too narrow. That is a BE change outside this FE-only PR;
I'll track the two together.
--
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]
