yyanyy commented on PR #57865: URL: https://github.com/apache/spark/pull/57865#issuecomment-5288619160
> Thanks for the PR, @yyanyy! > > This closes the write-side half of [SPARK-58389](https://issues.apache.org/jira/browse/SPARK-58389): every catalog-backed batch write target now loads through `CatalogV2Util.getTableForWrite`, so the option bag that reaches `LogicalWriteInfo` also reaches `loadTable(ident, context, options)`, and the internal write-privilege marker no longer lowercases the user's keys on its way through `UnresolvedRelation`. The shape looks right to me and CI is green (29 success, 1 skipped). My blocking findings are all about behaviour that changes beyond "options for writes" and isn't declared: two undeclared changes (`RelationResolution.loadRelation` now forwards options on the **read** re-resolution paths; the `SupportsCatalogOptions` save path now requests write privileges where it requested none), plus one measured break -- `writeTo(...).option("versionAsOf", ...)` now raises an analysis error where it used to be ignored. One release-scope question while `branch-4.3` is still open: #57582 landed there too, so as it stands 4.3 ships `loadTable(ident, context, options)` (`@since 4.3.0`) honored for reads but not writes -- is this meant to follow it there? > > ### Blocking > * **1.** _Undeclared read-path change:_ `loadRelation` now forwards options for DataFrame temp views and transaction re-resolution -- both reads, and the exact path #57582 declared out of scope -- but neither the title nor the description mentions it. [inline: `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:517`] > * **2.** _New write-privilege request on the `SupportsCatalogOptions` save path:_ base called `catalog.loadTable(ident)` with no privileges; this asks for `INSERT`/`INSERT,DELETE`, so a catalog that authorizes writes can now reject a `save()` that used to succeed. Worth doing, but it needs to be in the description and the user-facing-change section. [inline: `sql/core/src/main/scala/org/apache/spark/sql/classic/DataFrameWriter.scala:181`] > * **3.** _`writeTo(...).option("versionAsOf", ...)` now fails:_ verified by running it -- `UNSUPPORTED_FEATURE.TIME_TRAVEL` on this head, no error with only `DataFrameWriterV2.scala` reverted to base. Undocumented, and `insertInto`/`saveAsTable` still ignore the same option. [inline: `sql/core/src/main/scala/org/apache/spark/sql/classic/DataFrameWriterV2.scala:233`] > > ### Non-blocking > * **4.** _Pre-existing: a time-travelled source loses its pin inside a transaction:_ reproduced on this head **and on base** (writes the latest rows, not the pinned ones), so not caused by this PR -- but this is the method it touches, and a one-line guard fixes it. Happy for it to be a separate ticket. [inline: `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:514`] > * **5.** _Key-casing guarantee isn't covered end to end:_ the only mixed-case fixture asserts through the case-insensitive `get`, so nothing between `UnresolvedRelation` and `LogicalWriteInfo` is pinned. [inline: `sql/core/src/test/scala/org/apache/spark/sql/connector/AppendDataTransactionSuite.scala:53`] > * **6.** _`TxnTableCatalog` override drops the context:_ it records and then calls `loadTable(ident)`, so a time-travel load inside a transaction silently resolves to the latest table -- which is what keeps finding 4 invisible to this suite. [inline: `sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/txns.scala:206`] Thanks Peter for the detailed review! Addressed all comments. For 4.3, yes this is intended to be part of 4.3 ship too as with this we can complete the picture of option passing in the same release version. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
