[SPARK-51646][SQL] Fix propagating collation in views with default collation #50436

ilicmarkodb · 2025-03-27T22:53:30Z

What changes were proposed in this pull request?

Fixed propagating default collation to literals, subqueries, etc., in CREATE VIEW ... DEFAULT COLLATION ... query.
The issue was that the saved string used to construct the view did not include the DEFAULT COLLATION ... clause, resulting in the view being created without collation information.

Why are the changes needed?

Bug fix.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tests added to DefaultCollationTestSuite.

Was this patch authored or co-authored using generative AI tooling?

No.

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala

cloud-fan · 2025-03-31T08:56:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

+    // and does not include the DEFAULT COLLATION part, resulting in a plan without collation.
+    val plan = if (metadata.collation.isDefined) {
+      val newType = StringType(metadata.collation.get)
+      ResolveDDLCommandStringTypes.transformPlan(parsedPlan, newType)


Does this work for unresolved parsed plan? I feel it's better to match the View node and transform its child to resolve collation in the rule ResolveDDLCommandStringTypes, which is similar to how we match DDL/DML commands.

I think that created View doesn't pass through ResolveDDLCommandStringTypes. I tried to catch it with debugger.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ViewResolution.scala

Line 24 in a73ca47

object ViewResolution {

When resolving a View, we only resolve its child, meaning the View itself does not go through ResolveDDLCommandStringTypes.
Also, I think that this is clear way to resolve the problem because we will have correct plan from the moment of creating View.

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala

MaxGekk

lgtm except of minor comments

MaxGekk · 2025-04-04T09:20:34Z

sql/core/src/test/scala/org/apache/spark/sql/collation/DefaultCollationTestSuite.scala

+           | (c1)
+           | DEFAULT COLLATION sr_ai
+           | AS SELECT 'Ć' as c1 WHERE 'Ć' = 'C'
+           |""".stripMargin)


Should 2 spaces indentation, please, tune your IDE. Also see https://github.com/databricks/scala-style-guide?tab=readme-ov-file#indent

MaxGekk · 2025-04-04T09:21:14Z

sql/core/src/test/scala/org/apache/spark/sql/collation/DefaultCollationTestSuite.scala

+        s"""CREATE VIEW $testView DEFAULT COLLATION UTF8_LCASE
+           | as SELECT 'a' as c1
+           |""".stripMargin)


ditto: indentation

MaxGekk · 2025-04-04T13:01:29Z

+1, LGTM. Merging to master/4.0.
Thank you, @ilicmarkodb and @cloud-fan @dejankrak-db @stefankandic for review.

…llation ### What changes were proposed in this pull request? Fixed propagating default collation to literals, subqueries, etc., in `CREATE VIEW ... DEFAULT COLLATION ...` query. The issue was that the saved string used to construct the view did not include the `DEFAULT COLLATION` ... clause, resulting in the view being created without collation information. ### Why are the changes needed? Bug fix. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests added to `DefaultCollationTestSuite`. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50436 from ilicmarkodb/fix_subquery_literals_in_views_with_default_collation. Authored-by: ilicmarkodb <marko.ilic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit babb950) Signed-off-by: Max Gekk <max.gekk@gmail.com>

…llation ### What changes were proposed in this pull request? Fixed propagating default collation to literals, subqueries, etc., in `CREATE VIEW ... DEFAULT COLLATION ...` query. The issue was that the saved string used to construct the view did not include the `DEFAULT COLLATION` ... clause, resulting in the view being created without collation information. ### Why are the changes needed? Bug fix. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests added to `DefaultCollationTestSuite`. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50436 from ilicmarkodb/fix_subquery_literals_in_views_with_default_collation. Authored-by: ilicmarkodb <marko.ilic@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

vladimirg-db · 2025-04-10T14:16:24Z

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala

    // Also, table DEFAULT COLLATION cannot be specified through CREATE TABLE AS SELECT command.
    case _: V2CreateTablePlan | _: ReplaceTable | _: CreateView | _: AlterViewAs => true
+    // Check if view has default collation
+    case _ if AnalysisContext.get.collation.isDefined => true


I think this implementation is quite wrong, because now we match all the views from SELECT queries in ResolveDDLCommandStringTypes.

@cloud-fan can we reconsider this implementation?

do you mean we should only do it if the collation is not the default?

The naming is DDL specific (isDDLCommand, isCreateOrAlterPlan), but the rule actually changes plans of queries over collated views as well.

Do you mean nested views? every time we resolve a view we set this thread-local collation so it shouldn't be messed up.

We can fix the naming issues later but we do need to apply view default collation to view queries.

vladimirg-db · 2025-04-10T14:17:25Z

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala


+      // Check if view has default collation
+      case _ if AnalysisContext.get.collation.isDefined =>
+        StringType(AnalysisContext.get.collation.get)


@ilicmarkodb can you please implement this in the single-pass Analyzer? Thanks!

github-actions bot added the SQL label Mar 27, 2025

dejankrak-db approved these changes Mar 28, 2025

View reviewed changes

stefankandic reviewed Mar 28, 2025

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala Show resolved Hide resolved

ilicmarkodb added 4 commits March 30, 2025 18:08

fix

792a58e

stylefix

235b447

suggestions

b292bd4

temp

6af22b3

ilicmarkodb force-pushed the fix_subquery_literals_in_views_with_default_collation branch from 4237ea6 to 6af22b3 Compare March 30, 2025 16:17

cloud-fan reviewed Mar 31, 2025

View reviewed changes

ilicmarkodb requested a review from cloud-fan March 31, 2025 21:55

ilicmarkodb added 2 commits April 1, 2025 15:51

fix

b919fd1

fix

1162009

cloud-fan reviewed Apr 1, 2025

View reviewed changes

...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveDDLCommandStringTypes.scala Outdated Show resolved Hide resolved

cloud-fan approved these changes Apr 1, 2025

View reviewed changes

fix

93e00a9

MaxGekk reviewed Apr 4, 2025

View reviewed changes

ilicmarkodb added 2 commits April 4, 2025 11:53

scalastyle

39aaa3a

scalastyle

b6efb53

MaxGekk approved these changes Apr 4, 2025

View reviewed changes

MaxGekk closed this in babb950 Apr 4, 2025

vladimirg-db reviewed Apr 10, 2025

View reviewed changes

[SPARK-51646][SQL] Fix propagating collation in views with default collation #50436

[SPARK-51646][SQL] Fix propagating collation in views with default collation #50436

Conversation

ilicmarkodb commented Mar 27, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Uh oh!

cloud-fan Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilicmarkodb Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan Mar 31, 2025 •

edited

Loading

ilicmarkodb Mar 31, 2025 •

edited

Loading