[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/11171 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183434335 Thanks - I've merged this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user gatorsmile commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183423770 It is easy to rebase. After the merge, I will submit PRs for the other related JIRAs. Thanks! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183257659 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/51178/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183257656 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183257408 **[Test build #51178 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51178/consoleFull)** for PR 11171 at commit [`cfccf21`](https://github.com/apache/spark/commit/cfccf213a1bb89f68553f883c205e6b626f5be67). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183227971 **[Test build #51178 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51178/consoleFull)** for PR 11171 at commit [`cfccf21`](https://github.com/apache/spark/commit/cfccf213a1bb89f68553f883c205e6b626f5be67). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-183226591 Updated. cc @gatorsmile unfortunately you will have to rebase your pull requests, although it should be easy to do. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714855
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/SQLBuilderTest.scala ---
@@ -50,10 +50,8 @@ abstract class SQLBuilderTest extends QueryTest with
TestHiveSingleton {
""".stripMargin)
}
-val actualSQL = maybeSQL.get
-
try {
- assert(actualSQL === expectedSQL)
+ assert(generatedSql === expectedSQL)
--- End diff --
but i'm going to change this one to be consistent with expectedSQL
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714791
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/SQLBuilderTest.scala ---
@@ -50,10 +50,8 @@ abstract class SQLBuilderTest extends QueryTest with
TestHiveSingleton {
""".stripMargin)
}
-val actualSQL = maybeSQL.get
-
try {
- assert(actualSQL === expectedSQL)
+ assert(generatedSql === expectedSQL)
--- End diff --
i think both are acceptable according to java conventions. if the acronym
is longer than certain characters, than the preference is to camel case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714801
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/SQLBuilderTest.scala ---
@@ -50,10 +50,8 @@ abstract class SQLBuilderTest extends QueryTest with
TestHiveSingleton {
""".stripMargin)
}
-val actualSQL = maybeSQL.get
-
try {
- assert(actualSQL === expectedSQL)
+ assert(generatedSql === expectedSQL)
--- End diff --
`generatedSQL`? Do we have any naming rules related to acronyms (`Sql` or
`SQL`)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714696
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/SQLBuilderTest.scala ---
@@ -50,10 +50,8 @@ abstract class SQLBuilderTest extends QueryTest with
TestHiveSingleton {
""".stripMargin)
}
-val actualSQL = maybeSQL.get
-
try {
- assert(actualSQL === expectedSQL)
+ assert(generatedSql === expectedSQL)
--- End diff --
`generatedSQL`? Do we have any naming rules related to acronyms (`Sql` or
`SQL`)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714530
--- Diff:
sql/hive/src/main/scala/org/apache/spark/sql/hive/SQLBuilder.scala ---
@@ -37,157 +39,137 @@ import
org.apache.spark.sql.execution.datasources.LogicalRelation
class SQLBuilder(logicalPlan: LogicalPlan, sqlContext: SQLContext) extends
Logging {
def this(df: DataFrame) = this(df.queryExecution.analyzed, df.sqlContext)
- def toSQL: Option[String] = {
+ def toSQL: String = {
val canonicalizedPlan = Canonicalizer.execute(logicalPlan)
-val maybeSQL = try {
- toSQL(canonicalizedPlan)
-} catch { case cause: UnsupportedOperationException =>
- logInfo(s"Failed to build SQL query string because:
${cause.getMessage}")
- None
-}
-
-if (maybeSQL.isDefined) {
+try {
+ val generatedSQL = toSQL(canonicalizedPlan)
logDebug(
s"""Built SQL query string successfully from given logical plan:
- |
- |# Original logical plan:
- |${logicalPlan.treeString}
- |# Canonicalized logical plan:
- |${canonicalizedPlan.treeString}
- |# Built SQL query string:
- |${maybeSQL.get}
+|
+|# Original logical plan:
+|${logicalPlan.treeString}
+|# Canonicalized logical plan:
+|${canonicalizedPlan.treeString}
+|# Generated SQL:
+|$generatedSQL
""".stripMargin)
-} else {
+ generatedSQL
+} catch { case NonFatal(e) =>
logDebug(
s"""Failed to build SQL query string from given logical plan:
- |
+|
|# Original logical plan:
- |${logicalPlan.treeString}
- |# Canonicalized logical plan:
- |${canonicalizedPlan.treeString}
+|${logicalPlan.treeString}
+|# Canonicalized logical plan:
+|${canonicalizedPlan.treeString}
--- End diff --
Nit: Indentation is off.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714504
--- Diff:
sql/hive/src/main/scala/org/apache/spark/sql/hive/SQLBuilder.scala ---
@@ -37,157 +39,137 @@ import
org.apache.spark.sql.execution.datasources.LogicalRelation
class SQLBuilder(logicalPlan: LogicalPlan, sqlContext: SQLContext) extends
Logging {
def this(df: DataFrame) = this(df.queryExecution.analyzed, df.sqlContext)
- def toSQL: Option[String] = {
+ def toSQL: String = {
val canonicalizedPlan = Canonicalizer.execute(logicalPlan)
-val maybeSQL = try {
- toSQL(canonicalizedPlan)
-} catch { case cause: UnsupportedOperationException =>
- logInfo(s"Failed to build SQL query string because:
${cause.getMessage}")
- None
-}
-
-if (maybeSQL.isDefined) {
+try {
+ val generatedSQL = toSQL(canonicalizedPlan)
logDebug(
s"""Built SQL query string successfully from given logical plan:
- |
- |# Original logical plan:
- |${logicalPlan.treeString}
- |# Canonicalized logical plan:
- |${canonicalizedPlan.treeString}
- |# Built SQL query string:
- |${maybeSQL.get}
+|
+|# Original logical plan:
+|${logicalPlan.treeString}
+|# Canonicalized logical plan:
+|${canonicalizedPlan.treeString}
+|# Generated SQL:
+|$generatedSQL
""".stripMargin)
-} else {
+ generatedSQL
+} catch { case NonFatal(e) =>
logDebug(
s"""Failed to build SQL query string from given logical plan:
- |
+|
|# Original logical plan:
- |${logicalPlan.treeString}
- |# Canonicalized logical plan:
- |${canonicalizedPlan.treeString}
+|${logicalPlan.treeString}
+|# Canonicalized logical plan:
+|${canonicalizedPlan.treeString}
""".stripMargin)
+ throw e
}
-
-maybeSQL
}
- private def projectToSQL(
- projectList: Seq[NamedExpression],
- child: LogicalPlan,
- isDistinct: Boolean): Option[String] = {
-for {
- childSQL <- toSQL(child)
- listSQL = projectList.map(_.sql).mkString(", ")
- maybeFrom = child match {
-case OneRowRelation => " "
-case _ => " FROM "
- }
- distinct = if (isDistinct) " DISTINCT " else " "
-} yield s"SELECT$distinct$listSQL$maybeFrom$childSQL"
- }
+ private def toSQL(node: LogicalPlan): String = node match {
+case Distinct(p: Project) =>
+ projectToSQL(p, isDistinct = true)
- private def aggregateToSQL(
- groupingExprs: Seq[Expression],
- aggExprs: Seq[Expression],
- child: LogicalPlan): Option[String] = {
-val aggSQL = aggExprs.map(_.sql).mkString(", ")
-val groupingSQL = groupingExprs.map(_.sql).mkString(", ")
-val maybeGroupBy = if (groupingSQL.isEmpty) "" else " GROUP BY "
-val maybeFrom = child match {
- case OneRowRelation => " "
- case _ => " FROM "
-}
+case p: Project =>
+ projectToSQL(p, isDistinct = false)
-toSQL(child).map { childSQL =>
- s"SELECT $aggSQL$maybeFrom$childSQL$maybeGroupBy$groupingSQL"
-}
- }
+case p: Aggregate =>
+ aggregateToSQL(p)
+
+case p: Limit =>
+ s"${toSQL(p.child)} LIMIT ${p.limitExpr.sql}"
- private def toSQL(node: LogicalPlan): Option[String] = node match {
-case Distinct(Project(list, child)) =>
- projectToSQL(list, child, isDistinct = true)
-
-case Project(list, child) =>
- projectToSQL(list, child, isDistinct = false)
-
-case Aggregate(groupingExprs, aggExprs, child) =>
- aggregateToSQL(groupingExprs, aggExprs, child)
-
-case Limit(limit, child) =>
- for {
-childSQL <- toSQL(child)
-limitSQL = limit.sql
- } yield s"$childSQL LIMIT $limitSQL"
-
-case Filter(condition, child) =>
- for {
-childSQL <- toSQL(child)
-whereOrHaving = child match {
- case _: Aggregate => "HAVING"
- case _ => "WHERE"
-}
-conditionSQL = condition.sql
- } yield s"$childSQL $whereOrHaving $conditionSQL"
-
-case Union(children) if children.length > 1 =>
- val childrenSql = children.map(toSQL(_))
- if (childrenSql.exists(_.isEmpty)) {
-None
- } else {
-Some(childrenSql.map(_.get).mkStri
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11171#discussion_r52714511
--- Diff:
sql/hive/src/main/scala/org/apache/spark/sql/hive/SQLBuilder.scala ---
@@ -37,157 +39,137 @@ import
org.apache.spark.sql.execution.datasources.LogicalRelation
class SQLBuilder(logicalPlan: LogicalPlan, sqlContext: SQLContext) extends
Logging {
def this(df: DataFrame) = this(df.queryExecution.analyzed, df.sqlContext)
- def toSQL: Option[String] = {
+ def toSQL: String = {
val canonicalizedPlan = Canonicalizer.execute(logicalPlan)
-val maybeSQL = try {
- toSQL(canonicalizedPlan)
-} catch { case cause: UnsupportedOperationException =>
- logInfo(s"Failed to build SQL query string because:
${cause.getMessage}")
- None
-}
-
-if (maybeSQL.isDefined) {
+try {
+ val generatedSQL = toSQL(canonicalizedPlan)
logDebug(
s"""Built SQL query string successfully from given logical plan:
- |
- |# Original logical plan:
- |${logicalPlan.treeString}
- |# Canonicalized logical plan:
- |${canonicalizedPlan.treeString}
- |# Built SQL query string:
- |${maybeSQL.get}
+|
+|# Original logical plan:
+|${logicalPlan.treeString}
+|# Canonicalized logical plan:
+|${canonicalizedPlan.treeString}
+|# Generated SQL:
+|$generatedSQL
--- End diff --
Nit: Please revert the indentation change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-182821712 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/51093/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-182821707 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-182820588 **[Test build #51093 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51093/consoleFull)** for PR 11171 at commit [`9fd34fc`](https://github.com/apache/spark/commit/9fd34fc1fa27c09cfa5426a53be85cbc5e0460c3). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-182792045 **[Test build #51093 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51093/consoleFull)** for PR 11171 at commit [`9fd34fc`](https://github.com/apache/spark/commit/9fd34fc1fa27c09cfa5426a53be85cbc5e0460c3). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/11171#issuecomment-182790954 cc @liancheng --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-13282][SQL] LogicalPlan toSql should ju...
GitHub user rxin opened a pull request: https://github.com/apache/spark/pull/11171 [SPARK-13282][SQL] LogicalPlan toSql should just return a String Rather than Option[String]. Previously we were using Option[String] and None to indicate the case when Spark fails to generate SQL. It is easier to just use exceptions to propagate error cases, rather than having for comprehension everywhere. I also introduced a "build" function that simplifies string concatenation (i.e. no need to reason about whether we have an extra space or not). You can merge this pull request into a Git repository by running: $ git pull https://github.com/rxin/spark SPARK-13282 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/11171.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #11171 commit 9fd34fc1fa27c09cfa5426a53be85cbc5e0460c3 Author: Reynold Xin Date: 2016-02-11T10:08:15Z [SPARK-13282][SQL] LogicalPlan toSql should just return a String rather than Option[String] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
