924060929 commented on PR #65851:
URL: https://github.com/apache/doris/pull/65851#issuecomment-5099452687
### Follow-up: how other engines split the two defaults, and what that
suggests for the FE model
Not a blocker — this is about the shape of the abstraction, since the two
write-path issues I filed look like symptoms of one modelling decision. I
checked Iceberg 1.10.1 bytecode, the Iceberg Spark connector, Trino's Iceberg
plugin, and the spec.
#### 1. The library only covers the read half
In `iceberg-{api,core,parquet}-1.10.1`, `initialDefault` is referenced by
readers, `writeDefault` by no writer at all:
```
initialDefault -> SchemaUpdate, Schema, SchemaParser, NestedField,
UnionByNameVisitor,
avro/ValueReaders,
data/parquet/BaseParquetReaders$ReadBuilder <- reader
writeDefault -> SchemaUpdate, SchemaParser, NestedField,
UnionByNameVisitor <- metadata only
```
The read half is ~20 lines inside the reader builder (decompiled from
`BaseParquetReaders$ReadBuilder.defaultReader`):
```java
if (reader != null) return reader; // column present
in the file
if (field.initialDefault() != null) {
return ParquetValueReaders.constant(
convertConstant(field.type(), field.initialDefault()),
definitionLevel);
}
if (field.isOptional()) { ... } // else NULL /
required fails
```
It is that short because it sits where the `NestedField` is already in hand.
Doris pays a structural cost here that Spark and Trino do not — the reader is
in BE (C++) while the schema lives in FE (Java) — so this PR has to rebuild it
across a Thrift boundary. That part is unavoidable. The write half is on the
engine in every case.
#### 2. Nobody lets the two defaults share one slot
| | `initial-default` (read) | `write-default` (write) | statement-scoped
schema carrier |
|---|---|---|---|
| Iceberg lib | reader `constant` | metadata only | — |
| Spark | `withExistenceDefaultValue` → `EXISTS_DEFAULT` |
`withCurrentDefaultValue` → `CURRENT_DEFAULT` → `ResolveDefaultColumns` |
`SparkTable` (one instance) |
| Trino | page source `transforms.constantValue(...)` |
`ColumnMetadata.defaultValue` | `IcebergTableHandle` (`snapshotId` +
`tableSchemaJson`) |
Spark just wires the two Iceberg defaults onto the two default slots Spark
already had:
```java
// iceberg spark/v4.0/.../spark/TypeToSparkType.java
if (field.writeDefault() != null) {
sparkField =
sparkField.withCurrentDefaultValue(Literal$.MODULE$.create(writeDefault,
type).sql());
}
if (field.initialDefault() != null) {
sparkField =
sparkField.withExistenceDefaultValue(Literal$.MODULE$.create(initialDefault,
type).sql());
}
```
Trino has no such pair, so it routes them to two different layers.
`write-default` is the only one that reaches column metadata; `initial-default`
never does:
```java
// IcebergUtil.getColumnMetadatas(Schema schema, ...) — recomputed from the
Iceberg Schema every call
.setNullable(column.isOptional())
.setDefaultValue(formatIcebergDefaultAsSql(column.writeDefault(),
column.type()))
// IcebergPageSourceProvider — initial-default only as an execution-time
constant
Object initialDefault = getInitialDefault(tableSchema,
column.getBaseColumnIdentity().getId());
transforms.constantValue(writeNativeValue(column.getType(), initialDefault));
```
Also worth noting: both engines derive nullability straight from
`field.isOptional()`, so neither needs a separate required-field-id set
alongside the defaults.
#### 3. Where `Column.defaultValue` sits in that picture
Doris uses one slot for both halves. FE treats it as the write default
(`BindSink`, `InsertUtils.generateDefaultExpression`), and BE treats it as the
read fallback, filling from the **query-time** schema rather than the schema
the segment was written with:
```cpp
// be/src/storage/segment/segment.cpp:900
if (!_column_meta_accessor->has_column_uid(unique_id)) {
RETURN_IF_ERROR(new_default_iterator(tablet_column, iter));
return Status::OK();
}
```
The spec draws exactly the line this single slot cannot express (L388-389):
`initial-default` *must be set when a field is added and cannot change*,
`write-default` *may change*.
So `Column.defaultValue` is structurally the `write-default` slot — and it
is the direct counterpart of Trino's `ColumnMetadata.defaultValue`, down to
being an SQL string. The pre-PR code put `initial-default` there, which is what
made both `INSERT` (should use `write-default`) and `DESC` (Trino shows
`write-default`) wrong. Splitting them is right. My concern is only about what
replaced it.
#### 4. Suggestion: give `TableIf` a default provider instead of emptying
the slot
Today `IcebergUtils.parseField` stops populating `Column.defaultValue` but
the callers that still read it are not redirected anywhere, so they silently
degrade — `RewriteDefaultExpression` yields NULL for `DEFAULT(c)` in
UPDATE/MERGE-MATCHED, and `DESC` shows nothing. Trino never hits this because
it never stored a default anywhere else; there is exactly one entry point.
A shape that would fit Doris:
```java
Optional<Expression> writeDefaultOf(Column c); // INSERT omitted column,
DEFAULT(c), MERGE NOT MATCHED, DESC
Optional<Expression> readDefaultOf(Column c); // column absent from the
file/segment
```
- internal tables: both return `column.getDefaultValue()` — `TabletSchema`
keeps serializing it, nothing changes
- Iceberg: `writeDefaultOf` → pinned schema's `field.writeDefault()`,
`readDefaultOf` → `field.initialDefault()`
- every caller (`BindSink`, `InsertUtils`, `RewriteDefaultExpression`,
`FileScanNode`, `DESC`) asks the interface
That would fold in several loose ends at once: the UPDATE/MERGE `DEFAULT(c)`
regression and the missing `DESC` default both disappear; the three
`FileScanNode` hooks (`hasDefaultValue` / `getDefaultValueExpression` /
`isColumnAllowNull`, which callers must combine in a specific order and which
can disagree) collapse into one call returning a complete answer; and `DESC`
would show `write-default`, matching Trino.
#### 5. On pinning the schema — to be clear, I am not arguing against it
Spark pins (`SparkTable`, one instance shared by analyzer and writer) and
Trino pins (`IcebergTableHandle`, serialized to workers).
`IcebergWriteSchemaContext` should exist. The narrower point is that both
engines carry **one** instance with a single owner and therefore need no
reconciliation, whereas here the same object is stored in the unbound sink,
`StatementContext`, the logical sink, the physical sink,
`IcebergInsertCommandContext` and the planner sink, kept in agreement by three
different checks (`Preconditions` in `pinIcebergWriteSchema`, the `equals`
guard in `bindDataSink`, and `validateCurrentSchema`). Consolidating on
`StatementContext` as the single owner would also make the `refresh()`-related
issue easier to reason about, since there would be one place that decides which
schema this statement sees.
References: [spec
L326-337/L388-394](https://github.com/apache/iceberg/blob/main/format/spec.md),
[TypeToSparkType](https://github.com/apache/iceberg/blob/main/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/TypeToSparkType.java),
[SparkTable](https://github.com/apache/iceberg/blob/main/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java),
[trino
IcebergUtil](https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java),
[trino
IcebergPageSourceProvider](https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java).
--
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]
