[SPARK-58111][SQL] Scan and write schema narrowing for column-level UPDATE in DSv2 - #55518
[SPARK-58111][SQL] Scan and write schema narrowing for column-level UPDATE in DSv2#55518anuragmantri wants to merge 7 commits into
Conversation
fb14c34 to
ae635f4
Compare
| val required = | ||
| AttributeSet(dataAttrs) ++ AttributeSet(Seq(cond)) ++ AttributeSet(rowIdAttrs) | ||
| val narrowOutput = relation.output.filter(required.contains) | ||
| relation.copy(table = table, output = dedupAttrs(narrowOutput ++ rowIdAttrs ++ metadataAttrs)) |
There was a problem hiding this comment.
Can an attribute in required be missing from relation.output?
rowIdAttrs seems to be added 2 times.
If we already have a dedupAttrs() then probably doesn't make sense build AttributeSets.
There was a problem hiding this comment.
Can an attribute in required be missing from relation.output?
No. dataAttrs come from the connector's requiredDataAttributes() which are resolved against relation (via V2ExpressionUtils.resolveRefs), so they're guaranteed to be present. The condition's referenced columns are also table columns from the user's WHERE clause. rowIdAttrs and metadataAttrs can be absent from relation.output (they're resolved separately), but they're not part of the filter. They're appended unconditionally afterward via dedupAttrs(narrowOutput ++ rowIdAttrs ++ metadataAttrs)
rowIdAttrs seems to be added 2 times. If we already have dedupAttrs() then probably doesn't make sense to build AttributeSets.
Agreed. I fixed it.
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Could you resolve the conflicts, @anuragmantri ?
ae635f4 to
a99bb2d
Compare
Thanks. I rebased and fixed the conflicts. |
| return new NamedReference[0]; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit. Remove redundant empty line.
| * including the columns being updated. If {@link #requiredDataAttributes()} returns an empty | ||
| * array, Spark sends only the non-identity assigned columns (heuristic path). | ||
| * | ||
| * @since 4.2.0 |
| * <p> | ||
| * When empty (the default), Spark falls back to sending only the non-identity assigned columns. | ||
| * | ||
| * @since 4.2.0 |
| val table = buildOperationTable(tbl, UPDATE, CaseInsensitiveStringMap.empty()) | ||
| val updatedCols = assignments.collect { | ||
| case Assignment(key: AttributeReference, value) | ||
| if !isIdentityAssignment(key, value) => |
There was a problem hiding this comment.
One liner doesn't violate the line-length rule, does it?
- case Assignment(key: AttributeReference, value)
- if !isIdentityAssignment(key, value) =>
+ case Assignment(key: AttributeReference, value) if !isIdentityAssignment(key, value) =>| // | ||
| // When dataAttrs is non-empty, the relation output is narrowed to include only columns | ||
| // required for a column-update write. When dataAttrs is empty, the full relation.output is | ||
| // preserved. |
There was a problem hiding this comment.
For function description, please follow the community style like the other code path.
/**
* ...
*/
| // When the connector supports column updates and declares required data attributes, | ||
| // the read relation is narrowed at analysis time so that | ||
| // GroupBasedRowLevelOperationScanPlanning uses only the needed columns for the scan. | ||
| // Otherwise the full relation output is used. |
There was a problem hiding this comment.
For function description, please follow the community style like the other code path.
/**
* ...
*/
| WriteDelta(writeRelation, cond, rowDeltaPlan, relation, projections, groupFilterCond) | ||
| } | ||
|
|
||
| // Builds the row delta projection for the column update path. |
There was a problem hiding this comment.
For function description, please follow the community style like the other code path.
/**
* ...
*/
| dataAttrsResolved(inRowAttrs) | ||
| } | ||
|
|
||
| // Validates the narrow-write-schema row projection output. |
There was a problem hiding this comment.
For function description, please follow the community style like the other code path.
/**
* ...
*/
| table.skipSchemaResolution || areCompatible(inRowAttrs, outRowAttrs) | ||
| table.skipSchemaResolution || | ||
| areCompatible(inRowAttrs, outRowAttrs) || | ||
| dataAttrsResolved(inRowAttrs) |
There was a problem hiding this comment.
nit. Please minimize the change of existing code as much as possible like the following.
table.skipSchemaResolution || areCompatible(inRowAttrs, outRowAttrs) ||
dataAttrsResolved(inRowAttrs)
| * is ignored and the full table row is sent (the default behavior). | ||
| * <p> | ||
| * When non-empty, the returned columns become the write schema in declared order. | ||
| * The connector must declare all columns it wants to receive, including the columns being |
There was a problem hiding this comment.
This is very strong assumption, but it seems that this PR didn't have a protection. May I ask if we have some kind of assertion or a test coverage for this?
There was a problem hiding this comment.
Each column the connector returns passes through V2ExpressionUtils.resolveRefs which throws AnalysisException if the column is non existent.
I added a test test("column-update: requiredDataAttributes throws AnalysisException for invalid column")
| // | ||
| // ColumnPruning observes exactly these references and narrows the physical scan accordingly. | ||
| // Connectors that need additional columns in the scan (e.g., partition columns for | ||
| // distribution) should declare them in requiredDataAttributes(). |
There was a problem hiding this comment.
IIUC, for the correctness, we need to throw AnalysisException if requiredDataAttributes is invalid.
There was a problem hiding this comment.
Each column the connector returns passes through V2ExpressionUtils.resolveRefs which throws AnalysisException if the column is non existent.
I added a test test("column-update: requiredDataAttributes throws AnalysisException for invalid column")
| // Connectors that need additional columns in the scan (e.g., partition columns for | ||
| // distribution) should declare them in requiredDataAttributes(). | ||
| // | ||
| // Note: AlignUpdateAssignments guarantees all assignment keys are top-level |
There was a problem hiding this comment.
Do we have a test coverage for this, AlignUpdateAssignments contract?
There was a problem hiding this comment.
I added a new test test("column-update: nested struct field update narrows to the root struct column") that updates an inner field in a struct, the AlignUpdateAssignment returns only the root key.
| * whether pk is already in the updated columns list and, if not, add it to | ||
| * requiredDataAttributes(). | ||
| * | ||
| * @since 4.2.0 |
| // build a plan to replace read groups in the table | ||
| val writeRelation = relation.copy(table = operationTable) | ||
| val projections = buildReplaceDataProjections(query, relation.output, metadataAttrs) | ||
| val query = updatedAndRemainingRowsPlan |
There was a problem hiding this comment.
This looks like duplications: Let's use one variable instead of mixing two variables, updatedAndRemainingRowsPlan and query.
There was a problem hiding this comment.
Done, used a single variable
| // GroupBasedRowLevelOperationScanPlanning needs explicit column declarations to narrow. | ||
| val rowAttrs: Seq[Attribute] = if (isNarrow) connectorDataAttrs else relation.output | ||
|
|
||
| (readRelation, rowAttrs) |
There was a problem hiding this comment.
Please return metadataAttrs too to avoid the following recomputation in the caller-side.
val metadataAttrs = resolveRequiredMetadataAttrs(relation, operationTable.operation)
There was a problem hiding this comment.
I changed this to return metadataAttrs too.
| // | ||
| // Works for both the full-scan and narrow-scan CoW paths. In the narrow case, | ||
| // readRelation.output is already restricted by buildCoWReadSetup, so projecting | ||
| // all plan.output gives the correct narrow write schema. |
There was a problem hiding this comment.
Use function description style.
| * | ||
| * @since 4.2.0 | ||
| */ | ||
| default boolean supportsColumnUpdates() { |
There was a problem hiding this comment.
Given the scope of this PR, shall we mention that DELETE and MERGE ignores this method?
| * | ||
| * @since 4.2.0 | ||
| */ | ||
| default NamedReference[] requiredDataAttributes() { |
There was a problem hiding this comment.
Given the scope of this PR, shall we mention that DELETE and MERGE ignores this method?
There was a problem hiding this comment.
Even though the scope of this PR is UPDATE only, we'd like this API to work for MERGE as well (DELETE doesn't benefit since it doesn't write data columns). I'm still assessing what it takes and will add a section in the SPIP on how it could be implemented.
Happy to add a "currently only consulted for UPDATE" note in the Javadoc for now and remove it when MERGE support lands.
|
I finished the first round review, @anuragmantri . |
There was a problem hiding this comment.
Thanks for the review @dongjoon-hyun. I addressed your comments and cleaned up some AI generated comments which were redundant.
| return new NamedReference[0]; | ||
| } | ||
|
|
||
|
|
| * including the columns being updated. If {@link #requiredDataAttributes()} returns an empty | ||
| * array, Spark sends only the non-identity assigned columns (heuristic path). | ||
| * | ||
| * @since 4.2.0 |
| * is ignored and the full table row is sent (the default behavior). | ||
| * <p> | ||
| * When non-empty, the returned columns become the write schema in declared order. | ||
| * The connector must declare all columns it wants to receive, including the columns being |
There was a problem hiding this comment.
Each column the connector returns passes through V2ExpressionUtils.resolveRefs which throws AnalysisException if the column is non existent.
I added a test test("column-update: requiredDataAttributes throws AnalysisException for invalid column")
| * whether pk is already in the updated columns list and, if not, add it to | ||
| * requiredDataAttributes(). | ||
| * | ||
| * @since 4.2.0 |
| // | ||
| // When dataAttrs is non-empty, the relation output is narrowed to include only columns | ||
| // required for a column-update write. When dataAttrs is empty, the full relation.output is | ||
| // preserved. |
| // Connectors that need additional columns in the scan (e.g., partition columns for | ||
| // distribution) should declare them in requiredDataAttributes(). | ||
| // | ||
| // Note: AlignUpdateAssignments guarantees all assignment keys are top-level |
There was a problem hiding this comment.
I added a new test test("column-update: nested struct field update narrows to the root struct column") that updates an inner field in a struct, the AlignUpdateAssignment returns only the root key.
| // | ||
| // ColumnPruning observes exactly these references and narrows the physical scan accordingly. | ||
| // Connectors that need additional columns in the scan (e.g., partition columns for | ||
| // distribution) should declare them in requiredDataAttributes(). |
There was a problem hiding this comment.
Each column the connector returns passes through V2ExpressionUtils.resolveRefs which throws AnalysisException if the column is non existent.
I added a test test("column-update: requiredDataAttributes throws AnalysisException for invalid column")
| dataAttrsResolved(inRowAttrs) | ||
| } | ||
|
|
||
| // Validates the narrow-write-schema row projection output. |
| table.skipSchemaResolution || areCompatible(inRowAttrs, outRowAttrs) | ||
| table.skipSchemaResolution || | ||
| areCompatible(inRowAttrs, outRowAttrs) || | ||
| dataAttrsResolved(inRowAttrs) |
| * | ||
| * @since 4.2.0 | ||
| */ | ||
| default NamedReference[] requiredDataAttributes() { |
There was a problem hiding this comment.
Even though the scope of this PR is UPDATE only, we'd like this API to work for MERGE as well (DELETE doesn't benefit since it doesn't write data columns). I'm still assessing what it takes and will add a section in the SPIP on how it could be implemented.
Happy to add a "currently only consulted for UPDATE" note in the Javadoc for now and remove it when MERGE support lands.
| .getOrElse { | ||
| throw new AnalysisException( | ||
| errorClass = "_LEGACY_ERROR_TEMP_3075", | ||
| messageParameters = Map( | ||
| "tableAttr" -> tableAttr.toString, | ||
| "scanAttrs" -> scanAttrs.mkString(","))) | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this is safe because condition-referenced columns are guaranteed to be in the scan. Please correct me if I'm wrong.
There was a problem hiding this comment.
No. Unfortunately, this PR should not remove this because the existing sanity check is used for other code path in the existing test cases. Please recover it.
I guess you may achieve your goal via the following. Please review and revise the following example for your purpose.
private def buildTableToScanAttrMap(
tableAttrs: Seq[Attribute],
scanAttrs: Seq[Attribute],
requiredAttrs: AttributeSet): AttributeMap[Attribute] = {
// Table attrs may be legitimately absent from a column-update narrowed scan, so map only
// those that have a matching scan attribute. Attrs referenced by the condition must always
// be present (computeNarrowReadAttrs keeps them in the scan); failing to map one would
// leave a dangling reference in the group filter, so keep the strict check for them.
val attrMapping = tableAttrs.flatMap { tableAttr =>
val matched = scanAttrs.find(scanAttr => conf.resolver(scanAttr.name, tableAttr.name))
if (matched.isEmpty && requiredAttrs.contains(tableAttr)) {
throw new AnalysisException(
errorClass = "_LEGACY_ERROR_TEMP_3075",
messageParameters = Map(
"tableAttr" -> tableAttr.toString,
"scanAttrs" -> scanAttrs.mkString(",")))
}
matched.map(scanAttr => tableAttr -> scanAttr)
}
AttributeMap(attrMapping)
}There was a problem hiding this comment.
Makes sense. Similar to other changes for column updates paths, I created a conditional method buildNarrowTableToScanAttrMap() which is called only during column updates and throws when any of the condition references are missing. My rationale is that the runtime filtering applies to only the filters so it is sufficient remap the filters only. Let me know if this understanding is incorrect.
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for updating, @anuragmantri .
BTW, I cannot find the vote for the mentioned SPIP. Does pass the vote officially, @anuragmantri ? For SPIP, we need an official vote result to move forward including merging something, don't we? (cc @huaxingao as the Shepherd of SPARK-56599 JIRA issue)
What changes were proposed in this pull request?
For SPIP: SPARK-56599
cc @aokolnychyi too because RowLevelOperation.java has been never changed since being added 4 years ago via the following.
|
Thanks for the review @dongjoon-hyun. For the SPIP, we are waiting for a few more maintainers to also review the design as well as the PR before going for a vote. |
e806004 to
4060cbf
Compare
| * @since 4.3.0 | ||
| */ | ||
| default NamedReference[] updatedColumns() { | ||
| return new NamedReference[0]; |
There was a problem hiding this comment.
Hm... Defaulting to an empty array is fragile. Instances of this type are never constructed by connectors. Can Spark always fill the right value so that we don't have to default here?
There was a problem hiding this comment.
Do we handle nested column update correctly? The doc must be clear and implementation consistent.
There was a problem hiding this comment.
Added docs for nested column updates.
| * required for a column-update write. When dataAttrs is empty, the full relation.output is | ||
| * preserved. | ||
| */ | ||
| protected def buildRelationWithAttrs( |
There was a problem hiding this comment.
Not a big fan of this change, it makes the method fragile. I would consider adding another one instead.
Something like buildNarrowRelationWithAttrs() or something similar.
There was a problem hiding this comment.
I added a buildNarrowRelationWithAttrs() version and kept the original unchanged.
| relation.copy(table = table, output = attrs) | ||
| rowIdAttrs: Seq[AttributeReference] = Nil, | ||
| dataAttrs: Seq[AttributeReference] = Nil, | ||
| cond: Expression = TrueLiteral): DataSourceV2Relation = { |
There was a problem hiding this comment.
Don't think cond should be passed here, I'd compute prior to calling.
There was a problem hiding this comment.
I added a narrow version of this method which has a different implementation.
| } | ||
| } | ||
|
|
||
| private def isIdentityAssignment(key: Attribute, value: Expression): Boolean = { |
There was a problem hiding this comment.
Does this handle nested columns?
There was a problem hiding this comment.
Yes, I added a test for this.
| * plan.output may be narrowed by buildReplaceDataReadRelation, so only columns present in the | ||
| * plan are projected. | ||
| */ | ||
| private def buildReplaceDataUpdateProjection( |
There was a problem hiding this comment.
Added a new method for narrow projection
| val updatedAndRemainingRowsPlan = Union(updatedRowsPlan, remainingRowsPlan) | ||
|
|
||
| // build a plan to replace read groups in the table | ||
| val writeRelation = relation.copy(table = operationTable) |
There was a problem hiding this comment.
Why don't we want to narrow down the expected schema in the write relation too in this case?
There was a problem hiding this comment.
Instead of changing logic in ReplaceData that validates against relation.output?
There was a problem hiding this comment.
I thought about narrowing the write relation here. I preferred not to because it works for UPDATE but when we implement MERGE later, this would not work and we would have to have the full write relation because INSERT rows will need it.
Also, if we narrow it here, the writeSchema() and updateSchema() would look the same making updateSchema() redundant for UPDATEs. I would like to have writeSchema() always represent full width and updateSchema() be the narrow width to be consistent.
4060cbf to
1aed4f3
Compare
06c482e to
ac4474b
Compare
|
@aokolnychyi - This is ready for another review. |
362e2ef to
3fa6d10
Compare
|
CC: @dongjoon-hyun and @peter-toth, since you last reviewed, I changed the following
Please take another look. It will be great to have this in by feature freeze for Spark 4.3. |
peter-toth
left a comment
There was a problem hiding this comment.
Took another look after the redesign, @anuragmantri -- SupportsColumnUpdates is now an opt-in mix-in, with LogicalWriteInfo.updateSchema() carrying the narrow row shape and DataWriter.writeUpdate(...) as the narrow write channel. Reconstructed it fresh: RewriteUpdateTable narrows the scan to [connector-declared + cond + RHS + partition] refs and builds a separate narrow update projection, while the write relation stays full-width and updateRowProjection validates against the connector-declared set. The UPDATE paths (delta MoR, delta split, CoW) and their test coverage look solid.
Nothing blocking from my end -- the items below are all non-blocking. The one thing I'd like to see closed before merge is @aokolnychyi's open thread on narrowing the write relation: you've explained the full-width choice, but he hasn't re-reviewed since the redesign, so it'd be good to land his sign-off on that direction (finding 1 sits downstream of it).
Non-blocking
info.schema()differs between the CoW and delta paths for the same UPDATE.buildReplaceDataProjections' narrow branch has no empty-outputsForInsertguard, unlikebuildWriteDeltaProjectionsright below it, so a column-update UPDATE builds a full-widthrowProjectionwith-1column ordinals and reportsschema()as the full table -- while the delta path reports an empty schema. Harmless today (never projected for UPDATE) but a latentget(-1)once MERGE/INSERT rows reuse this path. This is downstream of the full-width write-relation choice @aokolnychyi asked about. [inline:RewriteRowLevelCommand.scala:257]- Document the UPDATE-only scope of
updatedColumns(). OnlyRewriteUpdateTablefills it, so DELETE and MERGE report an empty array even though MERGE does update columns -- worth stating on the public method, as @dongjoon-hyun asked earlier. [inline:RowLevelOperationInfo.java:43]
Minor
- Javadoc typo. Stray
)and "is" -> "are" in therequiredDataAttributes()doc. [inline:SupportsColumnUpdates.java:40] - Title undersells the change. The PR narrows both the scan and the write, but the title says only "Write schema narrowing" -- suggest "scan and write schema narrowing", matching your own MimaExcludes comment.
| val rowProjection = if (updateRowAttrs.nonEmpty) { | ||
| val outputsForInsert = filterOutputs(outputs, | ||
| OPERATIONS_WITH_ROW -- Set(UPDATE_OPERATION, COPY_OPERATION)) | ||
| newLazyProjection(plan, outputsForInsert, rowAttrs) |
There was a problem hiding this comment.
This narrow branch doesn't guard an empty outputsForInsert, unlike buildWriteDeltaProjections right below (lines 294-296). For a column-update UPDATE there are no INSERT/REINSERT rows, so outputsForInsert is empty and rowAttrs is the full relation.output; newLazyProjection then resolves ordinals against the narrow query and returns -1 for every column absent from the narrow scan, and info.schema() ends up as the full table shape -- whereas the delta path reports an empty schema for the same UPDATE.
It's harmless today because this projection is never applied for UPDATE, but the -1 ordinals become a real row.get(-1) once MERGE/INSERT rows flow through this path. Mirroring the WriteDelta guard fixes both the inconsistency and the latent case:
| newLazyProjection(plan, outputsForInsert, rowAttrs) | |
| if (outputsForInsert.isEmpty) { | |
| ProjectingInternalRow(StructType(Nil), Nil) | |
| } else { | |
| newLazyProjection(plan, outputsForInsert, rowAttrs) | |
| } |
There was a problem hiding this comment.
+1 for the above comment.
There was a problem hiding this comment.
Thanks for catching this. I mirrored the guard in buildWriteDeltaProjections
| Command command(); | ||
|
|
||
| /** | ||
| * Returns the columns being updated by this operation. |
There was a problem hiding this comment.
updatedColumns() is only populated by RewriteUpdateTable; DELETE and MERGE fall through to the default Nil, so a connector running a MERGE sees an empty array even though columns are being updated. Worth stating the UPDATE-only scope on the public method (the note @dongjoon-hyun asked for earlier):
| * Returns the columns being updated by this operation. | |
| * Returns the columns being updated by this operation. Currently populated only for UPDATE; | |
| * DELETE and MERGE report an empty array. |
| * reported by {@link RowLevelOperationInfo#updatedColumns()} plus any columns needed for row | ||
| * lookup or routing, e.g. a primary key). | ||
| * <p> | ||
| * If any of the columns from {@link RowLevelOperationInfo#updatedColumns()}) is |
There was a problem hiding this comment.
Stray ) after updatedColumns()} and is -> are:
| * If any of the columns from {@link RowLevelOperationInfo#updatedColumns()}) is | |
| * If any of the columns from {@link RowLevelOperationInfo#updatedColumns()} are |
|
|
||
| test("column-update: analysis fails when assignment key is outside requiredDataAttributes") { | ||
| // Connector declares only [pk] but the user assigns to `id`. We enforce | ||
| // updatedColumns ⊆ requiredDataAttributes at analysis time (root-column granularity). |
There was a problem hiding this comment.
| // updatedColumns ⊆ requiredDataAttributes at analysis time (root-column granularity). | |
| // updatedColumns is a subset of requiredDataAttributes at analysis time | |
| // (root-column granularity). |
There was a problem hiding this comment.
Is this auto-generated sentence, @anuragmantri ? AFAIK, Apache Spark CI will fail for non-ASCII characters like this.
There was a problem hiding this comment.
Yes, this was auto generated. I rewrote it as per your suggestion.
| table: RowLevelOperationTable, | ||
| metadataAttrs: Seq[AttributeReference], | ||
| rowIdAttrs: Seq[AttributeReference] = Nil): DataSourceV2Relation = { | ||
|
|
There was a problem hiding this comment.
Please recover this empty line removal. This removal seems that a leftover when you recover the original method. @aokolnychyi 's request was to keep the original method unchanged completely.
| } else { | ||
| info.schema() | ||
| } | ||
| PartitionBasedNarrowReplaceData(configuredScan, narrowSchema, info.schema()) |
There was a problem hiding this comment.
| PartitionBasedNarrowReplaceData(configuredScan, narrowSchema, info.schema()) | |
| // info.schema() is empty for column-update UPDATE writes (no INSERT rows); | |
| // use the table schema to align any INSERT-tagged rows. | |
| PartitionBasedNarrowReplaceData(configuredScan, narrowSchema, schema) |
| val outputsWithRow = filterOutputs(outputs, OPERATIONS_WITH_ROW) | ||
| Some(newLazyProjection(plan, outputsWithRow, rowAttrs)) | ||
| } else { | ||
| Some(ProjectingInternalRow(StructType(Nil), Nil)) |
There was a problem hiding this comment.
Could we keep the last branch as None and adjust the comment?
| Some(ProjectingInternalRow(StructType(Nil), Nil)) | |
| None |
This else branch is unreachable for the column-update path, so the change here is an unnecessary behavior change to DELETE:
- The comment says the
Some(...)is needed for identity-only column updates, but that
case never reaches this branch: an identity-only UPDATE on aSupportsColumnUpdates
connector still has non-emptyconnectorDataAttrs(an emptyrequiredDataAttributes()
is already rejected withEMPTY_REQUIRED_DATA_ATTRIBUTES), so it always takes the first
branch (updateRowAttrs.nonEmpty). - The only caller that reaches this branch is
RewriteDeleteFromTable, which passes
rowAttrs = Niland noupdateRowAttrs. For DELETE, this changesrowProjectionfrom
NonetoSome(empty projection). The current consumers happen to be equivalent
(V2Writesuses.map(_.schema).getOrElse(StructType(Nil))andDeltaWritingSparkTask
uses.orNull), so nothing breaks today, but it silently alters the DELETE plan shape
for no benefit and widens the blast radius of this PR.
Reverting this branch to None keeps the DELETE path byte-for-byte identical to master.
There was a problem hiding this comment.
Thanks for the explanation. I moved it back to None and updated the comment.
| val table = buildOperationTable(tbl, UPDATE, r.options) | ||
| val updatedCols = assignments.collect { | ||
| case Assignment(key: AttributeReference, value) if !isIdentityAssignment(key, value) => | ||
| FieldReference(key.name) |
There was a problem hiding this comment.
FieldReference(key.name) goes through the parser — FieldReference.apply(String) is
LogicalExpressions.parseReference(column) — rather than wrapping the name as-is. So a
top-level column whose name contains a dot (e.g. CREATE TABLE t (`a.b` INT, ...)) gets
reported to the connector as a nested reference a.b instead of a single column named
a.b, and names containing backticks can even fail to parse.
Since key is an already-resolved AttributeReference at this point, there is no reason
to re-parse its name. This matters because the value flows into
RowLevelOperationInfo.updatedColumns(), which connectors use to size
requiredDataAttributes(), and it also feeds validateUpdatedColumnsSubset — a
mis-parsed reference would surface as a spurious validation failure or a wrong narrow
schema.
| FieldReference(key.name) | |
| FieldReference(Seq(key.name)) |
There was a problem hiding this comment.
Thanks for the pointer. Done.
There was a problem hiding this comment.
I finished another round of reviews. Please address the above review comments, @anuragmantri .
BTW, please avoid pinging someone who is irrelevant this PR.
|
Thanks for the reviews @dongjoon-hyun and @peter-toth. I addressed your comments. |
peter-toth
left a comment
There was a problem hiding this comment.
Thanks for addressing the review, @anuragmantri -- re-checked through 791bc516be4; my findings 1-4 are all resolved (empty outputsForInsert guard now mirrored in buildReplaceDataProjections, with the test connector aligning INSERT-tagged rows to the table schema; updatedColumns() UPDATE-only scope documented; the SupportsColumnUpdates javadoc typo; and the title now covers scan + write). I also read the code that changed since my last round -- the new buildNarrowTableToScanAttrMap split and the Some(empty) -> None restore in the delta row projection both look correct (the None matches master's DELETE behavior, and the narrow group-filter path is exercised by the "runtime group filtering data correctness" test). Nothing new from me.
What changes were proposed in this pull request?
For SPIP: SPARK-56599
This PR adds an opt-in DSv2 mix-in for connectors to receive narrow rows on column-level UPDATE, so connectors can avoid reading and writing full-table rows when only a subset of columns is being updated.
Public API additions (all
@Evolving, since 4.3.0):requiredDataAttributes() accordingly.
When a connector mixes in SupportsColumnUpdates, Spark's UPDATE rewrite (RewriteUpdateTable) narrows both sides of the plan:
Why are the changes needed?
Schema narrowing helps connectors request for only updated columns enabling efficient column-level updates of wide tables.
Does this PR introduce any user-facing change?
Yes, new public DSv2 connector APIs:
All are @evolving and opt-in with backward-compatible defaults. Connectors that do not mix in SupportsColumnUpdates see no behavior change.
How was this patch tested?
New tests in:
Was this patch authored or co-authored using generative AI tooling?
I used Claude Code to generate code and tests and manually reviewed the generated code.