ilicmarkodb commented on PR #56100: URL: https://github.com/apache/spark/pull/56100#issuecomment-4556383877
> **Re-review status**: 1 addressed (new V2 test for the create-with-collation sticky case), 0 remaining, 1 new (late catch — I missed it on the first pass; this one I'd like to see folded into this PR rather than a follow-up, since it's the direct downstream of the rewrite this PR removes). > > The new V2 test (`V2: ALTER VIEW after ALTER NAMESPACE DEFAULT COLLATION keeps the inherited collation`) is the correct mirror of the V1 sibling `testAlterViewWithSchemaLevelCollation(alterSchemaCollation = true)` and uses `withNamespace` for cleanup — thanks! > > Could you also fold the following cleanup into this PR? Removing the `AlterViewAs` rewrite means three other call sites are documenting (and one is plumbing) behavior that no longer exists, and leaving them in place will mislead the next reader: > > * `sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala` L239–L244 (`AlterViewAsCommand.collation` Scaladoc) and L321–L325 (`alterPermanentView` inline comment) — both still describe `ApplyDefaultCollation` "folding / filling the namespace default" into the view's collation. Needs to be rewritten in terms of the current behavior (or simply removed once the parameter goes away — see below). > * `sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala` L546–L550 — comment says `ApplyDefaultCollation` "rewrites that property to fill the namespace default". Same issue. > * `sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala` L538–L541 — Scaladoc on `viewInfoBuilderFrom` lists `ApplyDefaultCollation` as a consumer. After this PR it isn't; the other listed users (SET / UNSET TBLPROPERTIES, SCHEMA BINDING execs) are still valid, so just drop the `ApplyDefaultCollation` reference. > > The structural piece: `AlterViewAsCommand.collation: Option[String]` (views.scala L245) was introduced specifically to carry the `ApplyDefaultCollation`-rewritten value across analysis → exec. With the rewrite gone, `rpv.info.properties.get(PROP_COLLATION)` read at `ResolveSessionCatalog.scala` L552–L556 always equals `viewMeta.collation` loaded at run time in `alterPermanentView` — both trace back to `CatalogTable.collation`, which nothing mutates between analysis and exec. So `collation.orElse(viewMeta.collation)` at views.scala L326 collapses to `viewMeta.collation`, and the parameter + the `ResolveSessionCatalog` collation-extraction block can be deleted. Once that's gone, the views.scala L239 / L321 comments can disappear with it rather than being rewritten. > > Same JIRA, same commit — please include before merge. done, thanks Wenchen! -- 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]
