[core][spark][flink] Support sub-field-level data evolution for nested columns - #8334
Conversation
| public class NestedSubfieldMergeIntoActionITCase extends ActionITCaseBase { | ||
|
|
||
| @Override | ||
| public void before() throws IOException { |
There was a problem hiding this comment.
This override drops the @BeforeEach annotation from ActionITCaseBase.before(), so JUnit never runs the setup for this class. As a result warehouse/catalog are not initialized and ReadWriteTableTestUtil.init(warehouse) is not called; the new test class currently fails all five tests with NPE at the first sEnv.executeSql(...). Please add @BeforeEach here (as the other action ITs do) so both the base setup and init(warehouse) run before each test.
There was a problem hiding this comment.
Good catch, thanks! You're right — overriding before() without re-adding @BeforeEach means JUnit never runs the base setup, so warehouse/init(warehouse) were uninitialized. Fixed in 23766fd by adding @BeforeEach to the override.
| sEnv.executeSql( | ||
| buildDdl( | ||
| "T", | ||
| Arrays.asList("id INT", "nest ROW<a INT, inner ROW<x INT, y INT>>"), |
There was a problem hiding this comment.
After adding the missing @BeforeEach locally to let this test class initialize, this DDL still fails before reaching the assertion: Flink's parser treats inner as a keyword (SQL parse failed. Encountered "inner" at line 1, column 41). Please quote the nested field name (and the matching CAST(ROW(... ) AS ROW<...>) below) or use a non-keyword name, otherwise testUpdateDeeplyNestedSubFieldThrows cannot exercise the intended deeper-than-one-level validation.
There was a problem hiding this comment.
Thanks! inner collides with the Flink SQL reserved word and breaks DDL parsing. Renamed the nested sub-field inner → sub (in the DDL, the CAST(ROW(...)) and the SET target) in 23766fd, so testUpdateDeeplyNestedSubFieldThrows now reaches and exercises the deeper-than-one-level validation.
|
Thanks for the review @JingsongLi! Addressed both points in 23766fd:
Also fixed the |
| // (subset) ROW carrying only the updated sub-fields, which is not directly | ||
| // cast-compatible with the full target struct. Accept it when every source | ||
| // sub-field exists in the target struct with a compatible cast. | ||
| boolean partialStructCompatible = |
There was a problem hiding this comment.
This relaxation should be scoped to sub-field writes. Today it also accepts whole-column assignments, e.g. --matched_update_set T.nest=S.nest where the source S.nest is ROW<a> and the target is ROW<a,b>. partialStructCompatible returns true here, but writePaths is still just nest, so sourceType is built as the full target struct and the partial RowData is sent to a whole-struct write. That can fail at runtime or create an incomplete whole-struct file. Please keep whole-struct assignments on the normal full-type compatibility check, and only allow this subset check when the column is actually being written through dotted paths such as nest.a.
There was a problem hiding this comment.
Agreed — fixed in ef144ac. The partial-struct check is now gated on isSubFieldWrite(column) (i.e. the column actually has dotted write paths like nest.a). Whole-column assignments such as T.nest=S.nest stay on the full-type compatibility check, so a narrower source struct is rejected instead of being written as an incomplete whole-struct file.
| matched.add(field.name()); | ||
| if (wholeChildren.contains(field.name()) | ||
| || subPaths.isEmpty() | ||
| || !(field.type() instanceof RowType)) { |
There was a problem hiding this comment.
Could we reject dotted paths when the selected head is not a ROW? With the current branch, projectByPaths(Collections.singletonList("id.a")) falls into this arm and returns the whole id field. That makes invalid dotted writeCols look valid to callers such as the conflict checker, and in the Flink action an invalid SET target under a scalar can pass path resolution before failing later with a less helpful error. Since dotted paths now encode physical sub-fields, this should throw unless the head field is a ROW, or the whole path matched an exact top-level field name.
There was a problem hiding this comment.
Good point — fixed in ef144ac. projectByPaths now throws IllegalArgumentException when a dotted path's head field is not a ROW (e.g. id.a), instead of silently returning the whole id. Exact top-level matches (including column names that themselves contain a dot) are still selected whole. Added coverage in DataTypesTest#testProjectByPaths.
| createReader(dataSplit, rowRanges, info.actualReadType), info); | ||
| } | ||
|
|
||
| private DataEvolutionFileReader createUnionReader( |
There was a problem hiding this comment.
Thanks for your contribution!
This class has already been marked as
TODO: Optimize implementation of this class.I think current createUnionReader is already hard to comprehend, the modified single method have 300 rows and many complicated logic. Is there any way to extract a dedicated class for this nested-data-evolution scenario?
There was a problem hiding this comment.
Thanks for the review! Agreed — the nested-data-evolution assembly is what grew createUnionReader.
Plan: extract the planning logic (leaf-level matching + the tree-shaped assembly plan, the current Steps 1–4 plus the collectLeafIds/providerOf/findSubProvider helpers) into a dedicated, pure DataEvolutionReadPlanner that returns an immutable plan (rowOffsets/fieldOffsets/NestedField[] + the per-bunch read fields). createUnionReader then just resolves the bunch schemas and builds the readers from that plan, so it goes back to a thin shell. A nice side effect is that the planning logic becomes directly unit-testable instead of only through ITs.
For the broader pre-existing TODO: Optimize implementation of this class (the top-level read path, mergeRangesAndSort, etc.), I'd suggest keeping that as a separate follow-up PR so this one stays focused on the nested feature — and I'd be happy to take part in that optimization PR as well. Does this approach sound good to you?
| this.writeCols = writeType.getFieldNames(); | ||
| // writeCols carries (possibly nested) dotted paths, e.g. ["f0", "nest.a"]; a plain | ||
| // top-level name means the whole column, a dotted path means only that sub-field is written | ||
| this.writeCols = writeType.leafPaths(rowType); |
There was a problem hiding this comment.
This can persist writeCols such as nest.sub.x for a deeper partial struct, but the read path below only supports composing one nested level and later throws when the full row is read (DataEvolutionSplitRead rejects partially-written nested sub-fields deeper than one level). That means a caller using BatchTableWrite.withWriteType(table.rowType().projectByPaths(Collections.singletonList("nest.sub.x"))) can successfully commit a file that makes normal full-table reads fail afterwards. Please reject unsupported deeper dotted paths before writing/committing them, or extend the reader to compose them recursively.
There was a problem hiding this comment.
Fixed in 7211c70. RowType.leafPaths now fails fast (UnsupportedOperationException) when a partial struct is nested inside another partial struct (a path deeper than one level, e.g. nest.sub.x), so withWriteType rejects it before any such file can be written/committed — a low-level BatchTableWrite.withWriteType(projectByPaths(["nest.sub.x"])) now throws up front instead of committing a file that later breaks full-table reads. One-level partial writes (nest.a, or a whole sub-struct nest.sub under a partial nest) are unaffected. Added DataTypesTest#testLeafPaths coverage.
|
Please resolve conflicts. |
|
@zhuxiangyi This is indeed a very significant change. Can you describe in detail why your business cannot use top-level fields? |
|
@JingsongLi Thanks for the review. This is indeed a significant change, so let me describe our real use case in detail — and I'd love to hear your suggestions. Background. We have a wide feature/data-source cache table for our risk engine. The modeling groups fields of the same kind (one data-source response / one feature family) into a single Why we keep it nested instead of flattening. At this scale, ~22k top-level columns become hard to work with for us — the schema is serialized into every snapshot/manifest, columnar footer & per-column stats metadata grow (especially painful for the small incremental files data evolution produces), and engine planning/codegen cost rises noticeably; day-to-day schema evolution also gets unwieldy. Modeling "one data source = one struct" lets us manage a source as a unit and prune by group on read, which fits us better. If there's a better modeling approach here, I'm very open to it. Read pattern. This table is only read by primary key (row id), pulling one or more whole structs to feed the risk engine — no aggregation, no filtering, no sub-field predicate pushdown. So nesting has essentially no downside for our reads, and top-level column pruning already reads only the structs actually requested. Why we need sub-field-level updates. We backfill specific sub-fields inside a group over historical data (when a feature definition changes / data is fixed — e.g. recomputing 8 of the ~2599 features in one group), across large historical row ranges. With the existing top-level (whole-column) evolution, changing those few sub-fields forces rewriting the entire struct (up to ~2599 fields) across history — large write amplification; and when a group is maintained by multiple pipelines, whole-column rewrites also clobber each other. Sub-field-level writes aligned by row id let us write only the backfilled leaves and reassemble the rest from the original files by row id, which is exactly the pain point this PR targets. Known trade-offs. The feature currently supports one level of nesting, and partially-written struct files don't contribute that column's stats to pushdown — which doesn't affect our "point-read only, no pushdown" usage, but it is a limitation and I've noted it in the description. If you think there's a more suitable direction (either in modeling or in the implementation), I'm happy to discuss and adjust, and to add more docs/tests. |
|
This PR is super complicated. We can first perform some refactoring PRs to make the entire code path move in the direction of Field Id, so that top-level fields and nested fields are treated the same. |
|
Thanks @JingsongLi, that makes sense — moving the whole path to field id (so a nested leaf id is just another field id, and top-level vs nested are handled the same) is cleaner than the dotted-path approach here, and it also removes the name-ambiguity under rename. Happy to do this as a series of smaller PRs. Here's a concrete plan and the compatibility strategy. Phase 1 — refactor to field id (no new feature, behavior unchanged)
Phase 2 — the nested feature on top of the id-based path
Compatibility strategy
Reuse from this PR: the read-assembly ( Does this split and the dual-write compatibility approach look right to you? Any adjustments welcome. |
|
@zhuxiangyi Sounds cool to me! |
Records the columns written in a data file by field id in addition to the existing name-based writeCols. Field ids are stable across column renames and can address nested fields uniformly, so this is groundwork for moving the data-evolution read/write path from names to field ids (see apache#8334 discussion). - DataFileMeta: append a nullable _WRITTEN_FIELD_IDS ARRAY<INT> to SCHEMA and add writtenFieldIds() (default null); thread it through PojoDataFileMeta and the forAppend/create factories. - DataFileMetaSerializer: serialize/deserialize the new field, isNullAt-guarded so old manifests read it as null. - Add DataFileMetaWriteColsLegacySerializer freezing the previous 20-field layout; bump DataSplit (8->9) and CommitMessage (11->12) versions to dispatch old streams to it. - Writers dual-write writtenFieldIds (derived from writeCols field ids) alongside writeCols, so old readers keep working via writeCols. - Add DataEvolutionUtils.writtenFieldIds(file, schemaFetcher) resolving a file's written columns to ids (writtenFieldIds if present, else writeCols names -> ids), for consumers to switch to in a follow-up. Behavior is unchanged; adds compatibility tests for round-trip, the frozen legacy layout and new-stream/old-serializer forward reads.
e118d87 to
acfba8a
Compare
|
@JingsongLi The branch has been rebased onto the latest master to resolve a conflict with #9114 (which reworked evolutionStats's winner-selection logic around the same time as this PR). Kept #9114's restructured logic as-is and re-attached the comment explaining why a sub-field-level partial-struct file's type mismatch is intentionally treated as "no stats" there. CI is green on the rebased commits. Marked it ready for review — would appreciate another look when you have time. @steFaiz Following up on the createUnionReader complexity you flagged — the extraction is done. The leaf-level matching and nested assembly planning now live in a dedicated, pure DataEvolutionReadPlanner (with its own DataEvolutionReadPlannerTest), and DataEvolutionSplitRead#createUnionReader is back to being a thin shell that just resolves bunch schemas and builds readers from the plan. Would appreciate a look when you have a chance. |
JingsongLi
left a comment
There was a problem hiding this comment.
Requesting changes because the current patch can silently corrupt persisted nested data. I reproduced two failures under Spark 3: reversed leaf order swaps values across fields, and a copied NULL parent struct becomes a non-NULL struct containing NULL children. The inline comments describe these blockers and the additional schema-contract issues. Before this is shipped, please also document and enforce the mixed-version barrier: old readers cannot reconstruct files whose writeCols contain entries such as nest.a; every reader, writer, compactor, and maintenance job must be upgraded before the feature is enabled, and binary rollback is unsafe after such files have been committed.
| if (perAction.isEmpty || perAction.exists(_.isEmpty)) { | ||
| None | ||
| } else { | ||
| val union = perAction.flatten.flatten.map(_._1).distinct |
There was a problem hiding this comment.
[P1] Canonicalize the leaf order before constructing the write schema. union preserves MATCHED-action order, while prunedStructType and buildPrunedStruct emit fields in table-schema order. writePaths later reuses this action-ordered sequence, so the Spark row layout and Paimon's writeType disagree positionally. I reproduced this with nest<a,b,c> and two clauses updating c and then a: expected (10,x,100) / (200,y,40), but read back (100,x,10) / (40,y,200). This silently corrupts persisted data. Please canonicalize the paths once in schema order and use that exact sequence for the output struct, writePaths, and writeType; apply the same fix to the Spark 4 copy and add a reverse-action-order regression test.
There was a problem hiding this comment.
Thanks for catching this. Fixed by canonicalizing the changed-leaf set to schema declaration order once, so the output struct, writePaths and writeType all follow the same sequence. Spark 4 copy updated too.
Your repro shape mattered — a single clause with two assignments doesn't reproduce, because Spark's own assignment alignment rebuilds it in schema order first. The regression test uses two clauses as you described.
| prunedByExprId.get(attr.exprId) match { | ||
| case Some((paths, _)) => | ||
| val st = attr.dataType.asInstanceOf[StructType] | ||
| buildPrunedStruct(st, Nil, paths, p => passthroughExpr(attr, st, p)) |
There was a problem hiding this comment.
[P1] Preserve the parent struct's nullness on copy/passthrough paths. buildPrunedStruct always returns a non-null CreateNamedStruct; when attr is NULL, this converts the copied value into a non-NULL struct whose selected children are NULL. I reproduced this by matching two source rows, conditionally updating nest.a only for row 1, and leaving row 2's nest as NULL: row 2 reads back with nest IS NULL = false. Please guard this construction with the parent-null condition (for example, an If(IsNull(attr), typedNull, prunedStruct)) and add a regression where the NULL row is included in the touched merge range. The Spark 4 copy has the same issue.
There was a problem hiding this comment.
Good catch. Guarded in copyOutput, and in updateOutput only for the pure-passthrough case — applying it unconditionally there would make SET t.nest.a = 5 on a NULL nest silently drop the assignment. Both have regression tests; Spark 4 copy updated.
Your repro shape mattered here too: the row must be matched but skipped by its clause, otherwise it keeps its nullness from the base file and doesn't expose the bug.
| // plain top-level name) is selected whole; only split into head.tail for genuine nested | ||
| // sub-field paths that do not name a field directly. This keeps backward compatibility | ||
| // with the legacy exact-name project(List). | ||
| if (dot < 0 || fieldByName.containsKey(path)) { |
There was a problem hiding this comment.
[P1] This exact-name preference makes the persisted dotted-path encoding ambiguous. A legal schema can contain both a quoted top-level field named a.b and a struct a with child b. leafPaths serializes the nested leaf as the same string a.b, but this branch reconstructs it as the top-level field. I verified that the emitted path resolves to the wrong field ID. Readers, pruning, and conflict detection can consequently attribute a partial file to the wrong field. Please use an unambiguous escaped/versioned or field-ID-based encoding; at minimum, reject a nested write whenever its flattened path collides with a top-level name, and cover the reader and conflict-checker paths in tests.
There was a problem hiding this comment.
Agreed — I confirmed the emitted path resolves to the wrong field id.
This PR takes the "at minimum, reject" option: leafPaths now refuses a nested path whose fh a literal top-level name, and asks the user to rename one of the two. The read side isunchanged, so nothing on disk changes meaning.
I haven't switched to the escaped/versioned encoding here, since that changes the persisted writeCols format. Happy to do it in a follow-up, or in this PR if you'd rather not merge without it.
| } | ||
|
|
||
| /** Whether {@code part} contains every (recursively nested) field of {@code full}. */ | ||
| private static boolean coversFully(RowType part, RowType full) { |
There was a problem hiding this comment.
[P1] coversFully must also preserve recursive physical field order. With full nest<a INT,b STRING> and projectByPaths(["nest.b", "nest.a"]), the write type is nest<b,a>, but this method returns true, so leafPaths collapses the metadata to [nest]; reconstruction then produces nest<a,b>. I verified that the two types are not equal. Row sidecars are written with the original physical write schema but read with the schema reconstructed from writeCols, so this can swap fields or decode bytes using the wrong type. Please require ordered recursive layout equality; otherwise retain the ordered dotted leaves, and add a round-trip test with different leaf types.
There was a problem hiding this comment.
You're right, thanks. coversFully now compares field ids positionally and recurses, so a cct keeps its ordered leaves instead of collapsing to the bare column name.
Note DataTypesTest.testProjectByPathsMatchesLegacyProjectIncludingOrder had asserted the p assertion is updated. I also added a reversed-nested-projection read test, sinceDataEvolutionSplitRead calls leafPaths for its format cache key.
| structCompatible = | ||
| isSubFieldWrite(flinkColumn.getName()) | ||
| ? isCompatiblePartialStruct(sourceStruct, targetStruct) | ||
| : isFullyCompatibleStruct(sourceStruct, targetStruct); |
There was a problem hiding this comment.
[P1] Whole-struct assignments are validated by field name here but are still written positionally. For target ROW<a INT,b INT> and source ROW<b INT,a INT>, this check accepts SET T.nest = S.nest; the projection keeps the source struct unchanged, while sourceType is rebuilt from the target schema order at lines 323-329. The nested row therefore reaches the writer in source order but is interpreted as target order, silently storing a = source.b and b = source.a (extra source fields can misalign it as well). Please recursively rebuild/cast whole structs in target order, or reject any source struct whose ordered shape differs, and add reversed-order and extra-field tests.
There was a problem hiding this comment.
Fixed with the second option you offered — reject rather than rebuild. isFullyCompatibleSt arity and the same field name at each position, recursing into nested rows, so a reordered or wider source is refused instead of being written under the wrong names.
| * Also sets {@link #writePaths}. | ||
| */ | ||
| private List<String> buildExplicitProject() { | ||
| Map<String, String> changes = parseCommaSeparatedKeyValues(matchedUpdateSet); |
There was a problem hiding this comment.
[P2] Validate duplicate SET targets before converting the clause to a map. parseCommaSeparatedKeyValues returns a map, so T.nest.a = S.x, T.nest.a = S.y loses the first entry before the duplicate check below can see it, and the last RHS silently wins. Please parse into an ordered entry list (or otherwise retain duplicate keys), reject duplicates before grouping, and test both exact duplicates and equivalent qualified/unqualified targets.
There was a problem hiding this comment.
Good point. matchedUpdateSet is now scanned for duplicates before parseCommaSeparatedKeyVaap, with keys normalized through the existing parseTargetPath so T.nest.a and nest.a compareequal.
… data evolution Addresses the review on apache#8334. Six issues, all of which could store or read values under the wrong field: - Spark: the changed-leaf set kept WHEN MATCHED clause order while the output struct was laid out in schema order, so two clauses touching sub-fields in reverse order made writeType disagree with the physical layout and swapped values on read. The leaf set is now canonicalized to schema order once and reused for the output struct, writePaths and writeType. - Spark: buildPrunedStruct built a CreateNamedStruct unconditionally, turning a copied NULL struct into a non-null struct of NULL children. Guarded in copyOutput, and in updateOutput only when the action sets no leaf of the column, so an explicit SET on a previously NULL struct still materializes it. - RowType: a nested leaf path flattened to "a.b" could collide with a literal top-level field of the same name, which projectByPaths resolves to the wrong field. leafPaths now rejects such a write instead of encoding it ambiguously. - RowType: coversFully compared field presence but not order, so a complete-but-reordered struct collapsed to the bare column name and lost its physical layout. It now compares field ids positionally and recurses. - Flink: isFullyCompatibleStruct validated a whole-struct assignment by field name only while the write is positional, accepting a reordered or wider source that would be stored under the wrong names. It now requires matching arity and per-position names. - Flink: parseCommaSeparatedKeyValues collapses the SET list into a map, so a duplicate target lost the earlier entry before the duplicate check ran. Duplicates are now detected beforehand, normalizing qualified and unqualified forms of the same target. Also documents the mixed-version barrier on data-evolution.nested-field.enabled: every reader, writer, compactor and maintenance job must be upgraded before the option is enabled, and downgrading is unsafe once a file whose write columns contain a nested sub-field path has been committed. Both ordering and null regressions are covered by end-to-end MERGE INTO tests using the clause shapes that actually expose them; a single clause is normalized into schema order by Spark's own assignment alignment, and an unmatched row keeps its parent-struct nullness from the base file, so neither reproduces the bugs. Adds coverage for compaction over sub-field files and for adding a nested sub-field after such files exist. Updates a DataTypesTest assertion that had encoded the old order-insensitive coversFully behaviour.
… data evolution Addresses the review on apache#8334. Six issues, all of which could store or read values under the wrong field: - Spark: the changed-leaf set kept WHEN MATCHED clause order while the output struct was laid out in schema order, so two clauses touching sub-fields in reverse order made writeType disagree with the physical layout and swapped values on read. The leaf set is now canonicalized to schema order once and reused for the output struct, writePaths and writeType. - Spark: buildPrunedStruct built a CreateNamedStruct unconditionally, turning a copied NULL struct into a non-null struct of NULL children. Guarded in copyOutput, and in updateOutput only when the action sets no leaf of the column, so an explicit SET on a previously NULL struct still materializes it. - RowType: a nested leaf path flattened to "a.b" could collide with a literal top-level field of the same name, which projectByPaths resolves to the wrong field. leafPaths now rejects such a write instead of encoding it ambiguously. - RowType: coversFully compared field presence but not order, so a complete-but-reordered struct collapsed to the bare column name and lost its physical layout. It now compares field ids positionally and recurses. - Flink: isFullyCompatibleStruct validated a whole-struct assignment by field name only while the write is positional, accepting a reordered or wider source that would be stored under the wrong names. It now requires matching arity and per-position names. - Flink: parseCommaSeparatedKeyValues collapses the SET list into a map, so a duplicate target lost the earlier entry before the duplicate check ran. Duplicates are now detected beforehand, normalizing qualified and unqualified forms of the same target. Also documents the mixed-version barrier on data-evolution.nested-field.enabled: every reader, writer, compactor and maintenance job must be upgraded before the option is enabled, and downgrading is unsafe once a file whose write columns contain a nested sub-field path has been committed. Both ordering and null regressions are covered by end-to-end MERGE INTO tests using the clause shapes that actually expose them; a single clause is normalized into schema order by Spark's own assignment alignment, and an unmatched row keeps its parent-struct nullness from the base file, so neither reproduces the bugs. Adds coverage for compaction over sub-field files and for adding a nested sub-field after such files exist. Updates a DataTypesTest assertion that had encoded the old order-insensitive coversFully behaviour.
e1cf1f9 to
1cf180c
Compare
… data evolution Addresses the review on apache#8334. Six issues, all of which could store or read values under the wrong field: - Spark: the changed-leaf set kept WHEN MATCHED clause order while the output struct was laid out in schema order, so two clauses touching sub-fields in reverse order made writeType disagree with the physical layout and swapped values on read. The leaf set is now canonicalized to schema order once and reused for the output struct, writePaths and writeType. - Spark: buildPrunedStruct built a CreateNamedStruct unconditionally, turning a copied NULL struct into a non-null struct of NULL children. Guarded in copyOutput, and in updateOutput only when the action sets no leaf of the column, so an explicit SET on a previously NULL struct still materializes it. - RowType: a nested leaf path flattened to "a.b" could collide with a literal top-level field of the same name, which projectByPaths resolves to the wrong field. leafPaths now rejects such a write instead of encoding it ambiguously. - RowType: coversFully compared field presence but not order, so a complete-but-reordered struct collapsed to the bare column name and lost its physical layout. It now compares field ids positionally and recurses. - Flink: isFullyCompatibleStruct validated a whole-struct assignment by field name only while the write is positional, accepting a reordered or wider source that would be stored under the wrong names. It now requires matching arity and per-position names. - Flink: parseCommaSeparatedKeyValues collapses the SET list into a map, so a duplicate target lost the earlier entry before the duplicate check ran. Duplicates are now detected beforehand, normalizing qualified and unqualified forms of the same target. Also documents the mixed-version barrier on data-evolution.nested-field.enabled: every reader, writer, compactor and maintenance job must be upgraded before the option is enabled, and downgrading is unsafe once a file whose write columns contain a nested sub-field path has been committed. Both ordering and null regressions are covered by end-to-end MERGE INTO tests using the clause shapes that actually expose them; a single clause is normalized into schema order by Spark's own assignment alignment, and an unmatched row keeps its parent-struct nullness from the base file, so neither reproduces the bugs. Adds coverage for compaction over sub-field files and for adding a nested sub-field after such files exist. Updates a DataTypesTest assertion that had encoded the old order-insensitive coversFully behaviour.
1cf180c to
282fb9f
Compare
|
@JingsongLi Thanks for the detailed review — the reproductions saved a lot of time. All six comments are addressed, and the mixed-version barrier is documented on data-evolutevery reader, writer, compactor and maintenance job must be upgraded before enabling it, anddowngrading is unsafe once a file whose write columns contain a nested sub-field path has been committed.
|
| // reading | ||
| // it whole would request leaves it lacks, and one-level composition | ||
| // cannot prune deeper than this level yet | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
[P1] Preserve deep nested schema evolution. A direct child ROW can be only partially covered because the source file predates a newly added nullable nested field, not because that child was written through an unsupported deep partial path. For example, after adding payload.inner.y to files that contain only payload.inner.x, any overlapping top-level partial update sends the group through this planner and this branch makes full reads and compaction fail. When all physically present leaves of the child come from one bunch and the remaining leaves are absent everywhere, please read the child from that provider and let the existing schema-evolution mapping null-fill the missing leaves. Reserve this exception for an actual cross-provider deep split, and add a regression covering deep ADD COLUMN plus an unrelated overlapping partial update.
| val writePaths = updateColumnsSorted.flatMap { | ||
| attr => | ||
| prunedByExprId.get(attr.exprId) match { | ||
| case Some((paths, _)) => paths.map(p => (attr.name +: p).mkString(".")) |
There was a problem hiding this comment.
[P1] Make both Spark conflict rewriters understand these dotted write paths. A nested MERGE now persists entries such as nest.a, but DataEvolutionRowIdConflictRewriter later treats every entry as a top-level relation attribute and throws Cannot find column nest.a when concurrent compaction changes the row-id boundaries. In the reverse commit order, DataEvolutionCompactMergeConflictRewriter uses exact top-level-name containment, finds no updated field for nest.a, and cannot rebase the staged compact output. Please carry path-aware write types through both rewriters; the row-id path must overlay only the staged leaves onto the current struct so untouched siblings are not clobbered. Apply the same change to the Spark 4 copy and add nested variants of the existing concurrent-compaction tests.
| */ | ||
| private List<String> parseTargetPath(String target) { | ||
| List<String> segs = new ArrayList<>(Arrays.asList(target.split("\\."))); | ||
| if (segs.size() > 1 && segs.get(0).equals(targetTableName())) { |
There was a problem hiding this comment.
[P2] Reject ambiguous target paths before stripping the qualifier. If the target table or alias is payload and the schema contains both payload ROW<a ...> and a top-level column a, the documented unqualified nested target payload.a is normalized to [a] here and the action successfully updates the top-level a instead. Please resolve both interpretations against the target schema and reject the input when both are valid, requiring an explicit form such as payload.payload.a for the nested field or bare a for the top-level field.
Today the smallest evolvable unit is a top-level column, so changing one sub-field of a struct rewrites the whole column. This records a partial struct write as dotted paths in writeCols (e.g. "nest.a") and reassembles the struct across files on read, so updating one sub-field only writes that leaf. - RowType.projectByPaths / leafPaths convert between a partial nested type and its dotted paths, preserving field ids. Fields are emitted in the order the paths are given, exactly like project(List): that order is the physical column layout a data file records in its writeCols, so it must not be normalised to schema order. - DataEvolutionReadPlanner: pure, no-IO planning of the read layout, doing leaf-level matching and nested assembly. Extracting it keeps DataEvolutionSplitRead's reader building thin and makes the layout logic directly unit-testable. - DataEvolutionRow composes a struct whose sub-fields live in several source files; DataEvolutionFileReader carries the plan. - Row-id conflict detection and writeCols resolution work at leaf field id granularity, so a whole-struct write and a sub-field write of the same struct still conflict. Only one level of partial nesting is supported; deeper splits are rejected at write time so a file that later breaks full-table reads can never be committed. Gated by data-evolution.nested-field.enabled (default false).
Lets data_evolution_merge_into target a nested sub-field, e.g. --matched_update_set "T.nest.a=S.newa", writing an incremental file that contains only that leaf instead of rewriting the whole struct. Sub-fields are emitted in schema declaration order rather than SET-clause order: the write paths become the physical column layout of the file, and that layout should not depend on how the statement happens to be written. The projection values are built by walking the pruned struct, so they follow automatically. Whole-column assignments keep the full-type compatibility check; the relaxed partial-struct check applies only to columns actually written through dotted paths.
For a struct column whose SET only touches some sub-fields, prune the aligned update to the changed leaves and write just those; the rest are copied from the target and reassembled on read. Falls back to a whole -column write whenever the changed leaves cannot be safely determined, so behaviour never regresses. Applied to the paimon-spark-4.0 copy of the class as well, which shadows the common one under the spark4 profile.
… data evolution Addresses the review on apache#8334. Six issues, all of which could store or read values under the wrong field: - Spark: the changed-leaf set kept WHEN MATCHED clause order while the output struct was laid out in schema order, so two clauses touching sub-fields in reverse order made writeType disagree with the physical layout and swapped values on read. The leaf set is now canonicalized to schema order once and reused for the output struct, writePaths and writeType. - Spark: buildPrunedStruct built a CreateNamedStruct unconditionally, turning a copied NULL struct into a non-null struct of NULL children. Guarded in copyOutput, and in updateOutput only when the action sets no leaf of the column, so an explicit SET on a previously NULL struct still materializes it. - RowType: a nested leaf path flattened to "a.b" could collide with a literal top-level field of the same name, which projectByPaths resolves to the wrong field. leafPaths now rejects such a write instead of encoding it ambiguously. - RowType: coversFully compared field presence but not order, so a complete-but-reordered struct collapsed to the bare column name and lost its physical layout. It now compares field ids positionally and recurses. - Flink: isFullyCompatibleStruct validated a whole-struct assignment by field name only while the write is positional, accepting a reordered or wider source that would be stored under the wrong names. It now requires matching arity and per-position names. - Flink: parseCommaSeparatedKeyValues collapses the SET list into a map, so a duplicate target lost the earlier entry before the duplicate check ran. Duplicates are now detected beforehand, normalizing qualified and unqualified forms of the same target. Also documents the mixed-version barrier on data-evolution.nested-field.enabled: every reader, writer, compactor and maintenance job must be upgraded before the option is enabled, and downgrading is unsafe once a file whose write columns contain a nested sub-field path has been committed. Both ordering and null regressions are covered by end-to-end MERGE INTO tests using the clause shapes that actually expose them; a single clause is normalized into schema order by Spark's own assignment alignment, and an unmatched row keeps its parent-struct nullness from the base file, so neither reproduces the bugs. Adds coverage for compaction over sub-field files and for adding a nested sub-field after such files exist. Updates a DataTypesTest assertion that had encoded the old order-insensitive coversFully behaviour.
…for sub-field data evolution Addresses the second review round on apache#8334. core: a direct child ROW that is only partially covered is the normal shape after a nested ADD COLUMN, not an unsupported deep partial write - a genuine cross-provider deep split is already rejected one branch earlier. Read the child whole from its single provider and let schema evolution null-fill the leaves the file predates. This also restores behaviour that worked before this PR: on master the union reader matched by top-level field id and handed the full read field to FormatReaderMapping, so any data-evolution table with a two-level struct, a deep ADD COLUMN and one partial update became unreadable. core: DataEvolutionSplitRead built its FormatReaderMapping cache key with RowType#leafPaths, which describes a *written* type relative to the schema it was written against and so enforces the write-side restrictions (at most one level of partial nesting). A read type is not bound by those: it may be pruned arbitrarily deep by the engine, and it may be wider than the file's own schema after a nested ADD COLUMN. Use a structural field-id key instead. This also fixes projecting a single leaf of a two-level struct, which was broken independently of ADD COLUMN. spark: both conflict rewriters treated every write column as a top-level relation attribute. DataEvolutionRowIdConflictRewriter threw "Cannot find column nest.a" on a concurrent compaction, and DataEvolutionCompactMergeConflictRewriter matched write columns by exact top-level name, so a MERGE file written as [value, nest.a] resolved to [value] alone and its nest.a update was silently dropped when the staged compact output was rebased. Both now resolve dotted paths, scan the top-level columns and project each partially written struct down to the leaves the file holds, so untouched siblings are not clobbered and a NULL struct stays NULL. flink: parseTargetPath stripped a leading table qualifier unconditionally. With a target table named "payload" holding both a struct column "payload" and a top-level "a", the documented target "payload.a" silently updated the top-level column. Resolve both readings against the target schema and reject the input when both are valid; when only one is valid, use it - the unqualified nested form now resolves instead of raising "invalid column reference".
282fb9f to
e83884d
Compare
DataEvolutionRowIdConflictRewriter builds two DataSourceV2Relations - one over the staged splits, one over the current ones - and handed both the same AttributeReference instances. That was harmless while every write column was a flat top-level name, because both scans were pruned to the same types. Once a write column addresses a struct leaf, nested column pruning rewrites the struct on one side only, leaving two attributes with the same expression id and different data types: Multiple attributes have the same expression ID 115 but different data types: STRUCT<a: INT, b: STRING>, STRUCT<a: INT> Spark 3.4+ reports this as an invalid plan and Spark 3.2/3.3 as a broken structural integrity check after V2ScanRelationPushDown. Call newInstance() per relation so the two scans never share expression ids. DataEvolutionCompactMergeConflictRewriter builds a single relation and is not affected.
Motivation
Local, high-frequency updates on a wide nested struct are expensive under today's data evolution: because the smallest evolvable unit is a top-level column, changing one sub-field (
nest.a) rewrites the entirenestcolumn — including all the unchanged sub-fields — into a new column-group file. This causes significant write amplification and storage waste exactly in the workloads that update structs most often. This PR lowers the column-group granularity to the leaf field, so "update one sub-field" only incrementally writes that sub-field, eliminating this class of write amplification at the root.Purpose
This PR pushes column-group granularity down to the leaf field: updating a single sub-field writes an incremental file containing only that leaf (a dotted write column like
nest.a), aligned by row id; on read the sub-fields scattered across files are reassembled into the full struct.Use cases — "wide nested struct + frequent local updates":
Local update of a user/entity profile. A row holds a wide
profile STRUCT<age, city, tags, last_login, score, ...>, but each operation only updates one or two sub-fields (login updateslast_login, risk-control updatesscore).profile(dozens of unchanged sub-fields).profile.last_loginincremental file is written, aligned by row id; the fullprofileis reassembled on read.Different pipelines/teams own different sub-fields of one struct. Pipeline A owns
nest.a, pipeline B ownsnest.b.Gated by a new table option
data-evolution.nested-field.enabled(defaultfalse); when disabled the behavior is identical to before (whole-column rewrite). Engine entries: SparkMERGE INTOand Flinkdata_evolution_merge_intoaction.Design (high level)
writeColsas dotted paths (nest.a) instead of only top-level names — noDataFileMetaserialization change. NewRowType.projectByPaths/leafPathsconvert between a (partial) nested type and its dotted paths, preserving field ids.writeCols.DataEvolutionSplitRead): match files at leaf field-id granularity and assemble a struct split across files sub-field by sub-field (latest-wins per leaf).DataEvolutionRowcomposes the struct from several source files.MergeIntoPaimonDataEvolutionTable): prune the aligned update to only the changed leaves; fall back to whole-column write when not safely determinable.DataEvolutionMergeIntoAction): parse dotted SET targets, rebuild a partial struct asCAST(ROW(...) AS ROW<...>), and write viaprojectByPaths. Reuses the existing top-level pipeline (row-id assign / shuffle / partial-write operator / commit).Tests
NestedDataEvolutionTableTest(5),NestedSubfieldDataEvolutionTableTest(3) — sub-field groups assembled, late overwrite, projection, compaction merges sub-fields.NestedSubfieldMergeIntoTest— single sub-field incremental write, whole-struct write, flag-off fallback.NestedSubfieldMergeIntoActionITCase(5) — single/multiple sub-fields (asserting dottedwriteCols), whole-struct, flag-off rejection, deeper-than-one-level rejection.API and Format
data-evolution.nested-field.enabled(Boolean, defaultfalse).DataFileMeta/ manifest format —writeColssemantics extended (a dotted entry means a written sub-field; a plain entry still means the whole column). Backward compatible with existing files.Documentation
docs/generated/core_configuration.htmlfor the new option.Limitations (follow-ups)
.are follow-ups.