[SPARK-57544][SQL] Rework column ID validation for nested fields in DSv2#56619
[SPARK-57544][SQL] Rework column ID validation for nested fields in DSv2#56619aokolnychyi wants to merge 1 commit into
Conversation
|
This is related to #55376. |
|
@gengliangwang @juliuszsompolski @andreaschat-db @longvu-db, what do you folks think? |
|
@huaxingao, this is the PR I was talking about on the dev list. |
|
This is not yet complete as it doesn't handle stripping column IDs on CTAS/RTAS. |
aokolnychyi
force-pushed
the
spark-57544
branch
2 times, most recently
from
0b3f26f to
30864e4
Compare
| .build(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Switching newly added create methods to a builder. Hasn't been released yet.
| * @param name the name of the column | ||
| * @param dataType the data type of the column | ||
| * @return a new builder | ||
| * @since 4.2.0 |
There was a problem hiding this comment.
Intentional to be cherry-picked into 4.2.
| return this; | ||
| } | ||
|
|
||
| public Builder metadataInJSON(String metadataInJSON) { |
There was a problem hiding this comment.
Probably call it metadata.
gengliangwang
left a comment
gengliangwang
left a comment
There was a problem hiding this comment.
2 blocking, 2 non-blocking, 1 nit.
Clean recursive generalization of the column-ID check, but two blockers: leftover references to the removed v1 machinery will fail tests, and the field-ID error message names the wrong (or no) field.
Design / architecture (2)
- Incomplete removal of the column-ID-v1 machinery:
DSv2IncrementallyConstructedQueryTests(run by the modifiedDataSourceV2DataFrameSuiteand byDataSourceV2DataFrameConnectSuite) still asserts the removedINCOMPATIBLE_TABLE_CHANGE_AFTER_ANALYSIS.COLUMN_ID_MISMATCHat lines 324/414/488 — the path now throwsCOLUMNS_MISMATCH, so thosecheckErrors fail.ComposedColumnIdTableCatalog(now unreferenced anywhere; it is the workaround this PR eliminates) andNullTableIdAndNullColumnIdInMemoryTableCatalogstill cite the removedvalidateColumnIdsin comments. Migrate the trait's assertions toCOLUMNS_MISMATCHand deleteComposedColumnIdTableCatalog. (likely overlaps the known "not yet complete" work) CatalogV2Util.scala:676:FIELD_IDmay ride into user-facingdf.schemametadata — see inline (question).
Correctness (2)
SchemaUtils.scala:453: field-ID error names the parent (or empty) path, not the field whose ID changed — masked by tests asserting the wrong output — see inline.SchemaUtils.scala:544:removeFieldIdsstruct branch always reallocates, contradicting its own Scaladoc — see inline.
Nits: 1 minor item (see inline comments).
PR description suggestions
- Document: the new mechanism (field IDs stored per-field in
StructFieldmetadata and validated withinvalidateSchemaCompatibility), not only the old approach's limitation — the "What changes" section is currently one line. - Document: the removal of the dedicated
validateColumnIdspass and theCOLUMN_ID_MISMATCHerror subclass; field-ID changes now report asCOLUMNS_MISMATCH.
| case Some(otherField) => | ||
| if (checkFieldIds) { | ||
| for (id <- field.id; otherId <- otherField.id if id != otherId) { | ||
| errors += s"${colPath.fullyQuoted} field ID has changed from $id to $otherId" |
There was a problem hiding this comment.
This reports the parent struct's path, not the field whose ID changed. The check runs one level above the recursion that appends field.name, so unlike the sibling errors (formatField for removed/added, and the type/nullability messages emitted inside the recursion) it omits the field name: a top-level column change becomes " field ID has changed from 1 to 99" (empty path), and a nested change names the container — two changed sibling fields then produce indistinguishable messages.
| errors += s"${colPath.fullyQuoted} field ID has changed from $id to $otherId" | |
| errors += s"${(colPath :+ field.name).fullyQuoted} field ID has changed from $id to $otherId" |
The six V2TableUtilSuite expectations (lines 569/587/602/621-622/639/656) assert the current output and will need updating; the explanatory comment at line 586 also carries a non-ASCII em-dash — switch it to ASCII while you're there.
| */ | ||
| def removeFieldIds(dataType: DataType): DataType = dataType match { | ||
| case s: StructType => | ||
| StructType(s.fields.map { field => |
There was a problem hiding this comment.
This branch always rebuilds via StructType(s.fields.map(...)), so it never returns the original s — but the Scaladoc promises "the original instance unchanged when there is nothing to strip," which only the ArrayType/MapType branches honor. InMemoryBaseTable.assignFieldIds (line 945) shows the struct short-circuit: track whether any field changed and return s if none did. Either apply that here or relax the comment.
| f = encodeDefaultValue(default, f) | ||
| } | ||
| Option(col.id()).foreach { id => | ||
| f = f.withId(id) |
There was a problem hiding this comment.
Encoding the top-level ID here also rides into the user-facing schema: DataSourceV2Relation.create builds its output from table.columns.asSchema (which calls this) via toAttributes, and FIELD_ID isn't in INTERNAL_METADATA_KEYS, so it looks like it ends up in df.schema field metadata for every column / nested field of an ID-tracking connector. Is that intended? If not, adding FIELD_ID to INTERNAL_METADATA_KEYS (or stripping it at the relation boundary) would keep it internal. I traced the encode path but didn't confirm end-user visibility — could you check?
| * IDs are preserved across type changes, keeping the same column ID through type | ||
| * widening and nested field additions. [[TypeChangeResetsColIdTableCatalog]] overrides | ||
| * this behavior for testing scenarios where type changes should produce a new ID. | ||
| * Existing IDs are preserved: Column → StructType → Column round-trip encodes them in |
There was a problem hiding this comment.
Non-ASCII arrows (U+2192); use ASCII (CLAUDE.md / scalastyle):
| * Existing IDs are preserved: Column → StructType → Column round-trip encodes them in | |
| * Existing IDs are preserved: Column -> StructType -> Column round-trip encodes them in |
2 participants
What changes were proposed in this pull request?
This PR reworks column ID validation for nested fields in DSv2.
Why are the changes needed?
The original implementation detected dropped-and-re-added columns by comparing top-level Column.id() strings in a dedicated validateColumnIds pass, but this approach had no visibility into nested struct fields, array elements, or map keys/values. To work around this limitation, connectors had to encode nested field IDs into the top-level ID string (as demonstrated by ComposedColumnIdTableCatalog), placing an unreasonable burden on connector authors and making the feature fragile by design.
Does this PR introduce any user-facing change?
Yes but it targets unreleased functionality and must be cherry picked to 4.2.
How was this patch tested?
Existing and new tests.
Was this patch authored or co-authored using generative AI tooling?
Claude Code v2.1.183