[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r657622253 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -432,10 +432,26 @@ class Analyzer(override val catalogManager: CatalogManager) case e if !e.resolved => u case g: Generator => MultiAlias(g, Nil) case c @ Cast(ne: NamedExpression, _, _, _) => Alias(c, ne.name)() -case e: ExtractValue => Alias(e, toPrettySQL(e))() +case e: ExtractValue => + val onlyExtract = e.collect { +case _: ExtractValue => true +case _ => false + }.forall(a => a) Review comment: let's write a small function for it: ``` def extractOnly(e: Expression): Boolean = e match { case _: ExtractValue => e.children.forall(extractOnly) case _: Literal => true case _: Attribute => true case _ => false } ``` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r657622410 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -432,10 +432,26 @@ class Analyzer(override val catalogManager: CatalogManager) case e if !e.resolved => u case g: Generator => MultiAlias(g, Nil) case c @ Cast(ne: NamedExpression, _, _, _) => Alias(c, ne.name)() -case e: ExtractValue => Alias(e, toPrettySQL(e))() +case e: ExtractValue => + val onlyExtract = e.collect { +case _: ExtractValue => true +case _ => false + }.forall(a => a) + if (onlyExtract) { +Alias(e, toPrettySQL(e))() + } else { +val meta = new MetadataBuilder() + .putString("autoGeneratedAlias", "true") Review comment: it's an internal metadata, let's name it `__ autoGeneratedAlias ` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r657623564 ## File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala ## @@ -534,6 +535,14 @@ object ViewHelper extends SQLConfHelper with Logging { } } + def verifyAutoGeneratedAliasesNotExists(child: LogicalPlan, name: TableIdentifier): Unit = { +child.expressions.foreach { Review comment: We should check `child.output`, instead of expressions. e.g. ``` SELECT * FROM (SELECT a + b FROM t) ``` The root plan here is a `Project` with `AttributeReference` only, and the check here can't catch it. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r659609709 ## File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala ## @@ -910,4 +910,45 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils { } } } + + test("SPARK-35686: error out for creating view with auto gen alias") { +withView("v") { + val e = intercept[AnalysisException] { +sql("CREATE VIEW v AS SELECT count(*) FROM VALUES (1), (2), (3) t(a)") + } + assert(e.getMessage.contains("without explicitly assigning an alias")) + sql("CREATE VIEW v AS SELECT count(*) AS cnt FROM VALUES (1), (2), (3) t(a)") + checkAnswer(sql("SELECT * FROM v"), Seq(Row(3))) +} + } + + test("SPARK-35686: error out for creating view with auto gen alias - nested type") { +withView("v") { + val e = intercept[AnalysisException] { +sql("CREATE VIEW v AS SELECT array(1, 2, 3)[0]") Review comment: This is not a very interesting case. How about testing `SELECT * FROM (SELECT a + b ...)`? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r659609709 ## File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala ## @@ -910,4 +910,45 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils { } } } + + test("SPARK-35686: error out for creating view with auto gen alias") { +withView("v") { + val e = intercept[AnalysisException] { +sql("CREATE VIEW v AS SELECT count(*) FROM VALUES (1), (2), (3) t(a)") + } + assert(e.getMessage.contains("without explicitly assigning an alias")) + sql("CREATE VIEW v AS SELECT count(*) AS cnt FROM VALUES (1), (2), (3) t(a)") + checkAnswer(sql("SELECT * FROM v"), Seq(Row(3))) +} + } + + test("SPARK-35686: error out for creating view with auto gen alias - nested type") { +withView("v") { + val e = intercept[AnalysisException] { +sql("CREATE VIEW v AS SELECT array(1, 2, 3)[0]") Review comment: This is not a very interesting case. How about testing a real nested case like `SELECT * FROM (SELECT a + b ...)`? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r659948827 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -424,6 +424,12 @@ class Analyzer(override val catalogManager: CatalogManager) */ object ResolveAliases extends Rule[LogicalPlan] { private def assignAliases(exprs: Seq[NamedExpression]) = { + def extractOnly(e: Expression): Boolean = e match { +case _: ExtractValue => e.children.forall(extractOnly) +case _: Literal => true Review comment: since we allow literal here, maybe we should allow it in the non `ExtractValue` case as well -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r659949686 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -424,6 +424,12 @@ class Analyzer(override val catalogManager: CatalogManager) */ object ResolveAliases extends Rule[LogicalPlan] { private def assignAliases(exprs: Seq[NamedExpression]) = { + def extractOnly(e: Expression): Boolean = e match { +case _: ExtractValue => e.children.forall(extractOnly) +case _: Literal => true Review comment: or maybe we shouldn't allow literal here either -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r659948827 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -424,6 +424,12 @@ class Analyzer(override val catalogManager: CatalogManager) */ object ResolveAliases extends Rule[LogicalPlan] { private def assignAliases(exprs: Seq[NamedExpression]) = { + def extractOnly(e: Expression): Boolean = e match { +case _: ExtractValue => e.children.forall(extractOnly) +case _: Literal => true Review comment: since we allow literal here, maybe we should allow it in the non `ExtractValue` case as well ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -424,6 +424,12 @@ class Analyzer(override val catalogManager: CatalogManager) */ object ResolveAliases extends Rule[LogicalPlan] { private def assignAliases(exprs: Seq[NamedExpression]) = { + def extractOnly(e: Expression): Boolean = e match { +case _: ExtractValue => e.children.forall(extractOnly) +case _: Literal => true Review comment: or maybe we shouldn't allow literal here either -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r661940720 ## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala ## @@ -424,6 +424,12 @@ class Analyzer(override val catalogManager: CatalogManager) */ object ResolveAliases extends Rule[LogicalPlan] { private def assignAliases(exprs: Seq[NamedExpression]) = { + def extractOnly(e: Expression): Boolean = e match { +case _: ExtractValue => e.children.forall(extractOnly) +case _: Literal => true Review comment: sure -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32832: [SPARK-35686][SQL] Not allow using auto-generated alias when creating view
cloud-fan commented on a change in pull request #32832: URL: https://github.com/apache/spark/pull/32832#discussion_r661988654 ## File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala ## @@ -910,4 +910,45 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils { } } } + + test("SPARK-35686: error out for creating view with auto gen alias") { Review comment: shall we put the tests in `PersistedViewTestSuite`? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org