[SPARK-57251][SDP] Validate SCD2 reserved framework columns at AutoCDC flow construction#57488
[SPARK-57251][SDP] Validate SCD2 reserved framework columns at AutoCDC flow construction#57488anew wants to merge 2 commits into
Conversation
…C flow construction AutoCdcMergeFlow only rejected source columns using the reserved AutoCDC prefix (`__spark_autocdc_`). SCD2 additionally persists the framework columns `__START_AT` and `__END_AT`, which do not carry that prefix, so a colliding source column would be silently overwritten during preprocessing. Add a constructor-time check that rejects source columns colliding (by exact name, resolver-aware) with SCD2's non-prefixed reserved framework columns. It runs before the flow schema is forced, so it surfaces this actionable error ahead of the temporary AUTOCDC_SCD2_NOT_SUPPORTED gate and remains correct once SCD2 support lands. SCD1 targets carry no non-prefixed framework columns, so the check is a no-op for SCD1. Adds the AUTOCDC_RESERVED_COLUMN_NAME_CONFLICT error condition and tests in AutoCdcFlowSuite. Co-authored-by: Isaac
anew
changed the title
|
LGTM, thank you @anew! |
| * The prefix guard above only rejects names starting with [[AutoCdcReservedNames.prefix]]. | ||
| * SCD2 additionally persists the framework columns [[Scd2BatchProcessor.startAtColName]] and | ||
| * [[Scd2BatchProcessor.endAtColName]], which do NOT carry that prefix, so a colliding source | ||
| * column would otherwise be silently overwritten during preprocessing (SPARK-57251). SCD1 |
There was a problem hiding this comment.
Nit; can we get rid of the SPARK JIRA in these comments (they show up quite a few times and dont bring up much value.) its quite easy now to see which pr did it.
And overall can we simplify the comments to reduce bloat
There was a problem hiding this comment.
done. This was Claude imitating other unit tests that also have the JIRA id, but you are right, that convention is now not really needed.
| * Note [[startAtColName]] and [[endAtColName]] do NOT carry the reserved | ||
| * [[AutoCdcReservedNames.prefix]], so a source-column collision with them is not caught by the | ||
| * prefix-based guard; [[org.apache.spark.sql.pipelines.graph.AutoCdcMergeFlow]] validates the | ||
| * source schema against this set at construction time (SPARK-57251) to fail fast with a |
There was a problem hiding this comment.
Nit: This says the flow validates against this entire set, but the new check filters it to the non-prefixed names; the prefixed member is handled separately by requireReservedPrefixAbsentInSourceColumns. Could we say "validates the source schema against the non-prefixed names in this set" for precision?
| case ScdType.Type1 => | ||
| Set.empty | ||
| } | ||
|
|
There was a problem hiding this comment.
Optional: find reports only the first collision, so if the source contains both __START_AT and __END_AT, the error identifies only one as present. Would it be useful to collect all conflicts and report them together? The current message does list all reserved names, and this matches the prefix guard above, so keeping this as-is also seems reasonable.
There was a problem hiding this comment.
Kept find as-is. You noted that keeping it "also seems reasonable" since it matches the prefix guard. Changing only this guard to collect-all would make it inconsistent with its sibling requireReservedPrefixAbsentInSourceColumns (which also uses find), and the error message already lists all reserved names.
…nces Per review feedback: - Remove the SPARK-57251 references from source comments and test names/section header (the merged PR already records provenance), and condense the surrounding comments to reduce bloat. - Clarify the reservedFrameworkColNames scaladoc to say the flow validates the source schema against the *non-prefixed* names in the set (the prefixed member is handled by requireReservedPrefixAbsentInSourceColumns), for precision. No behavior change; the reserved framework-column check still uses `find` (reporting the first collision), consistent with the sibling prefix guard. Co-authored-by: Opus 4.8
|
This PR is replaced by #57527. |
4 participants
What changes were proposed in this pull request?
What changes were proposed in this pull request?
AutoCdcMergeFlow validates at construction time that a flow's source change-data feed does not carry columns that collide with AutoCDC's internal columns. Until now, that validation (requireReservedPrefixAbsentInSourceColumns) only rejected column names starting with the reserved prefix _spark_autocdc.
SCD2, however, persists two framework columns to the target that do not carry that prefix — __START_AT and __END_AT (see Scd2BatchProcessor.reservedFrameworkColNames). A source column named __START_AT or __END_AT therefore slipped past the guard and would be silently overwritten during microbatch preprocessing.
This PR closes that gap (the TODO(SPARK-57251) in Scd2BatchProcessor):
Why are the changes needed?
Without this check, a user whose CDC source happens to contain a __START_AT or __END_AT column would have that data silently overwritten by AutoCDC's SCD2 framework columns, with no error and no diagnostic — a data-correctness footgun. Failing fast at flow construction with a user-actionable error ("rename or remove the column") is the intended UX, consistent with the existing reserved-prefix guard.
Does this PR introduce any user-facing change?
Yes. An AutoCDC SCD2 flow whose source change-data feed contains a column named __START_AT or __END_AT (subject to case-sensitivity settings) now fails at flow construction with AUTOCDC_RESERVED_COLUMN_NAME_CONFLICT instead of silently overwriting the column. There is no change for SCD1 flows, and no change for SCD2 flows whose sources do not use these names. (Note: SCD2 AutoCDC flows are not yet generally supported on master — they are still gated by AUTOCDC_SCD2_NOT_SUPPORTED — so no released behavior changes.)
How was this patch tested?
New unit tests in AutoCdcFlowSuite covering:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)