JingsongLi commented on PR #9822:
URL: https://github.com/apache/paimon/pull/9822#issuecomment-5738464956
Thanks for the feature. I reviewed the write and read paths end to end and
found two blocking issues plus a consistency concern.
### 1. [P1] The option is applied for every changelog producer, but only the
`lookup` path fills the extra fields
`MergeTreeCompactManagerFactory`'s constructor widens the changelog value
type whenever the option is set:
```java
RowType changelogValueType = computeChangelogValueType(valueType, options);
if (changelogValueType != null) {
writerFactoryBuilder.withChangelogValueType(changelogValueType);
}
```
There is no `changelog-producer == lookup` check here, and the option is not
validated anywhere else (docs/description only say "Only valid when
changelog-producer is lookup"). The extra fields are actually populated only by
the lookup merge-function wrapper (`preserveFieldIndices` is only passed to
`LookupMergeFunctionWrapperFactory` inside the lookup branch of
`createRewriter`).
Because the same `KeyValueFileWriterFactory.Builder` instance is shared with
`KeyValueFileStoreWrite` (the compact-manager factory is created in
`KeyValueFileStoreWrite`'s constructor, and `createWriter` later calls
`writerFactoryBuilder.build(...)`), the merge-tree writer's changelog format
context is widened to N+K for **all** producers:
- `changelog-producer='input'`: `MergeTreeWriter` flushes changelog records
built from the incoming values (`MergeTreeWriter.java:219-220` ->
`createRollingChangelogFileWriter`) whose value rows have N fields, but the
writer now expects N+K value fields.
- `changelog-producer='full-compaction'`: same for
`ChangelogMergeTreeRewriter.java:147`.
Result: the flush/compaction fails while serializing the shorter row (e.g.
`AIOOBE` in the row writer), or worse, corrupt changelog files are written.
Please gate the widening on `options.changelogProducer() ==
ChangelogProducer.LOOKUP` (and/or reject the option at schema validation for
other producers).
### 2. [P1] The widened Spark DSv2 schema breaks ordinary Spark writes to
such tables
`PaimonSparkTableBase.schema` (and `BaseScan.tableRowType`) add the K
metadata columns to the DSv2 table schema. Paimon tables advertise
`ACCEPT_ANY_SCHEMA`, so Spark's own `ResolveOutputRelation` is skipped and
writes are resolved by Paimon's `PaimonAnalysis` -> `PaimonOutputResolver`. For
a by-position write, `SchemaEvolutionHelper.expectedAttrsForCatalogWrite`
returns `table.output` (now N+K columns), and
`PaimonOutputResolver.resolveColumnsByPosition` throws on the size mismatch:
```
Cannot write to `t`, the number of data columns (3) doesn't match the table
schema's (4).
```
So on a table with `changelog-producer.expose-field-as-metadata` set, plain
Spark SQL writes such as `INSERT INTO t SELECT id, data, event_ts FROM src` or
`INSERT INTO t VALUES (...)` fail, even though the metadata columns are
generated rather than user input - the new test itself says "it is not an input
column". By-name writes happen to survive (top-level NULL fill), which makes
the failure mode even more confusing.
Additionally, `INSERT INTO t SELECT * FROM t` produces N+K columns, which
resolves positionally; with `spark.paimon.write.merge-schema` enabled,
`SchemaEvolutionHelper.commitSchemaEvolution` will merge `__internal__event_ts`
into the persisted table schema, because `SparkSystemColumns` only filters
`_bucket_`/`_row_kind_`.
Suggested fix: keep the generated metadata columns out of write-side
resolution, e.g. filter them in
`expectedAttrsForCatalogWrite`/`SparkSystemColumns`, or expose them only on the
scan side while keeping `PaimonSparkTableBase.schema` at the physical N columns.
### 3. [P2] Inconsistent unknown-column handling and duplicated extension
logic
- `MergeTreeCompactManagerFactory.computeChangelogValueType` throws
`IllegalArgumentException` for a configured column that does not exist, while
`KeyValueFileStore.newReaderFactoryBuilder` and
`ChangelogEventMetadataTable.computeExtendedRowType` silently skip it (`if (idx
>= 0)`). A typo in the option therefore fails at write/compaction time but is
ignored on the read path (metadata column silently NULL) - please pick one
behavior, ideally validating at schema/option level.
- The "append `prefix + name` fields with `max(id) + 1` and nullable type
copy" logic is duplicated in `MergeTreeCompactManagerFactory` (twice),
`KeyValueFileStore`, `ChangelogEventMetadataTable.computeExtendedRowType` and
again in a different variant in `MergeFileSplitRead`/`RawFileSplitRead`. These
must stay in sync for the reader mapping (`createMetadataFallbackMapping`) to
line up; a single helper would be safer.
Minor: for changelog files written before the option was enabled, the
metadata columns read as NULL (the fallback mapping is only applied to data
files, via `!isChangelogFile(file)`). Worth one sentence in the docs if that is
intended.
--
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]
