[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
