morningman commented on code in PR #69536:
URL: https://github.com/apache/doris/pull/69536#discussion_r4369757763
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2327,8 +2343,77 @@ 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; a rebuilt field keeps its
+ * initial default in the promoted type ({@link #promotedDefault}) and
drops its write default, which no reader
+ * uses and whose stale value may not fit the promoted type.
+ */
+ 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())) {
Review Comment:
Confirmed, and fixed in `b4bb074bfed`. The relaxation compared each retained
type with the type it had reached so far, so its result depended on the order
of the schema list; my earlier statement that it did not was wrong. This only
matters when a key has two retained promotions that do not promote to each
other: in v3, DATE to both TIMESTAMP and TIMESTAMP_NS, or `unknown` to two
types, and both can be retained only after the current schema is set back to an
earlier one (or after a type change that is not a valid promotion). Your
history lists DATE, TIMESTAMP, TIMESTAMP_NS, so the loop stopped at TIMESTAMP,
and FileScannerV2 rounded the 1 ns tombstone to midnight in DATETIMEV2(6) and
deleted the DATE row. Master's reverse lookup took TIMESTAMP_NS, so this PR
introduced the regression; V1 compares in the delete file's own type and was
not affected. The `unknown` rule from the previous round has the same flaw:
after `unknown`→INT, a rollback and `unknown`→STRING, the carrier was
INT and turned a non-numeric STRING tombstone into NULL, which matches every
row written while k was `unknown`.
No single carrier type is lossless for every such history, and the metadata
does not record which type a delete file was written in. TIMESTAMP drops
nanoseconds, and TIMESTAMP_NS cannot hold DATE or TIMESTAMP values outside
1677-09-21..2262-04-11, which FileScannerV2 rejects for DATE and turns into
NULL for TIMESTAMP. The carrier now takes the promotion that every other
retained promotion of the key promotes to, and planning fails with `Iceberg
equality-delete field 2 (k) was promoted to both timestamp and timestamp_ns in
schema history` when there is none. Only promotions of the definition the
lineage selected count, so a delete committed while TIMESTAMP_NS was current
still resolves TIMESTAMP_NS next to the abandoned TIMESTAMP schema. New tests
replay your history in both orders and the `unknown` variant, and cover that
lineage case.
One related history stays as on master and branch-4.2: if a delete is
prepared under a TIMESTAMP_NS schema that is then abandoned, and a snapshot is
committed while TIMESTAMP is current, the lineage records TIMESTAMP and the
carrier keeps it. Telling those apart needs the delete file's own key type,
which is the FileScannerV2 follow-up I'm tracking with the cleanup 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]
