You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spark.apache.org by we...@apache.org on 2016/08/18 08:17:39 UTC
spark git commit: [SPARK-17034][SQL] Minor code cleanup for
UnresolvedOrdinal
Repository: spark
Updated Branches:
refs/heads/master 10204b9d2 -> 3e6ef2e8a
[SPARK-17034][SQL] Minor code cleanup for UnresolvedOrdinal
## What changes were proposed in this pull request?
I was looking at the code for UnresolvedOrdinal and made a few small changes to make it slightly more clear:
1. Rename the rule to SubstituteUnresolvedOrdinals which is more consistent with other rules that start with verbs. Note that this is still inconsistent with CTESubstitution and WindowsSubstitution.
2. Broke the test suite down from a single test case to three test cases.
## How was this patch tested?
This is a minor cleanup.
Author: petermaxlee <pe...@gmail.com>
Closes #14672 from petermaxlee/SPARK-17034.
Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/3e6ef2e8
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/3e6ef2e8
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/3e6ef2e8
Branch: refs/heads/master
Commit: 3e6ef2e8a435a91b6a76876e9833917e5aa0945e
Parents: 10204b9
Author: petermaxlee <pe...@gmail.com>
Authored: Thu Aug 18 16:17:01 2016 +0800
Committer: Wenchen Fan <we...@databricks.com>
Committed: Thu Aug 18 16:17:01 2016 +0800
----------------------------------------------------------------------
.../spark/sql/catalyst/analysis/Analyzer.scala | 2 +-
.../analysis/SubstituteUnresolvedOrdinals.scala | 54 ++++++++++++++++
.../UnresolvedOrdinalSubstitution.scala | 52 ---------------
.../spark/sql/catalyst/planning/patterns.scala | 13 ----
.../SubstituteUnresolvedOrdinalsSuite.scala | 67 ++++++++++++++++++++
.../UnresolvedOrdinalSubstitutionSuite.scala | 65 -------------------
6 files changed, 122 insertions(+), 131 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
index f540816..cfab6ae 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
@@ -84,7 +84,7 @@ class Analyzer(
CTESubstitution,
WindowsSubstitution,
EliminateUnions,
- new UnresolvedOrdinalSubstitution(conf)),
+ new SubstituteUnresolvedOrdinals(conf)),
Batch("Resolution", fixedPoint,
ResolveRelations ::
ResolveReferences ::
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinals.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinals.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinals.scala
new file mode 100644
index 0000000..6d8dc86
--- /dev/null
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinals.scala
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.catalyst.CatalystConf
+import org.apache.spark.sql.catalyst.expressions.{Expression, Literal, SortOrder}
+import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LogicalPlan, Sort}
+import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.CurrentOrigin.withOrigin
+import org.apache.spark.sql.types.IntegerType
+
+/**
+ * Replaces ordinal in 'order by' or 'group by' with UnresolvedOrdinal expression.
+ */
+class SubstituteUnresolvedOrdinals(conf: CatalystConf) extends Rule[LogicalPlan] {
+ private def isIntLiteral(e: Expression) = e match {
+ case Literal(_, IntegerType) => true
+ case _ => false
+ }
+
+ def apply(plan: LogicalPlan): LogicalPlan = plan transform {
+ case s: Sort if conf.orderByOrdinal && s.order.exists(o => isIntLiteral(o.child)) =>
+ val newOrders = s.order.map {
+ case order @ SortOrder(ordinal @ Literal(index: Int, IntegerType), _) =>
+ val newOrdinal = withOrigin(ordinal.origin)(UnresolvedOrdinal(index))
+ withOrigin(order.origin)(order.copy(child = newOrdinal))
+ case other => other
+ }
+ withOrigin(s.origin)(s.copy(order = newOrders))
+
+ case a: Aggregate if conf.groupByOrdinal && a.groupingExpressions.exists(isIntLiteral) =>
+ val newGroups = a.groupingExpressions.map {
+ case ordinal @ Literal(index: Int, IntegerType) =>
+ withOrigin(ordinal.origin)(UnresolvedOrdinal(index))
+ case other => other
+ }
+ withOrigin(a.origin)(a.copy(groupingExpressions = newGroups))
+ }
+}
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitution.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitution.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitution.scala
deleted file mode 100644
index e21cd08..0000000
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitution.scala
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.spark.sql.catalyst.analysis
-
-import org.apache.spark.sql.catalyst.CatalystConf
-import org.apache.spark.sql.catalyst.expressions.{Expression, SortOrder}
-import org.apache.spark.sql.catalyst.planning.IntegerIndex
-import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LogicalPlan, Sort}
-import org.apache.spark.sql.catalyst.rules.Rule
-import org.apache.spark.sql.catalyst.trees.CurrentOrigin.withOrigin
-
-/**
- * Replaces ordinal in 'order by' or 'group by' with UnresolvedOrdinal expression.
- */
-class UnresolvedOrdinalSubstitution(conf: CatalystConf) extends Rule[LogicalPlan] {
- private def isIntegerLiteral(sorter: Expression) = IntegerIndex.unapply(sorter).nonEmpty
-
- def apply(plan: LogicalPlan): LogicalPlan = plan transform {
- case s @ Sort(orders, global, child) if conf.orderByOrdinal &&
- orders.exists(o => isIntegerLiteral(o.child)) =>
- val newOrders = orders.map {
- case order @ SortOrder(ordinal @ IntegerIndex(index: Int), _) =>
- val newOrdinal = withOrigin(ordinal.origin)(UnresolvedOrdinal(index))
- withOrigin(order.origin)(order.copy(child = newOrdinal))
- case other => other
- }
- withOrigin(s.origin)(s.copy(order = newOrders))
- case a @ Aggregate(groups, aggs, child) if conf.groupByOrdinal &&
- groups.exists(isIntegerLiteral(_)) =>
- val newGroups = groups.map {
- case ordinal @ IntegerIndex(index) =>
- withOrigin(ordinal.origin)(UnresolvedOrdinal(index))
- case other => other
- }
- withOrigin(a.origin)(a.copy(groupingExpressions = newGroups))
- }
-}
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala
index f42e67c..476c66a 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala
@@ -209,19 +209,6 @@ object Unions {
}
/**
- * Extractor for retrieving Int value.
- */
-object IntegerIndex {
- def unapply(a: Any): Option[Int] = a match {
- case Literal(a: Int, IntegerType) => Some(a)
- // When resolving ordinal in Sort and Group By, negative values are extracted
- // for issuing error messages.
- case UnaryMinus(IntegerLiteral(v)) => Some(-v)
- case _ => None
- }
-}
-
-/**
* An extractor used when planning the physical execution of an aggregation. Compared with a logical
* aggregation, the following transformations are performed:
* - Unnamed grouping expressions are named so that they can be referred to across phases of
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinalsSuite.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinalsSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinalsSuite.scala
new file mode 100644
index 0000000..3c429eb
--- /dev/null
+++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinalsSuite.scala
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.catalyst.analysis.TestRelations.testRelation2
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.dsl.plans._
+import org.apache.spark.sql.catalyst.expressions.Literal
+import org.apache.spark.sql.catalyst.SimpleCatalystConf
+
+class SubstituteUnresolvedOrdinalsSuite extends AnalysisTest {
+ private lazy val conf = SimpleCatalystConf(caseSensitiveAnalysis = true)
+ private lazy val a = testRelation2.output(0)
+ private lazy val b = testRelation2.output(1)
+
+ test("unresolved ordinal should not be unresolved") {
+ // Expression OrderByOrdinal is unresolved.
+ assert(!UnresolvedOrdinal(0).resolved)
+ }
+
+ test("order by ordinal") {
+ // Tests order by ordinal, apply single rule.
+ val plan = testRelation2.orderBy(Literal(1).asc, Literal(2).asc)
+ comparePlans(
+ new SubstituteUnresolvedOrdinals(conf).apply(plan),
+ testRelation2.orderBy(UnresolvedOrdinal(1).asc, UnresolvedOrdinal(2).asc))
+
+ // Tests order by ordinal, do full analysis
+ checkAnalysis(plan, testRelation2.orderBy(a.asc, b.asc))
+
+ // order by ordinal can be turned off by config
+ comparePlans(
+ new SubstituteUnresolvedOrdinals(conf.copy(orderByOrdinal = false)).apply(plan),
+ testRelation2.orderBy(Literal(1).asc, Literal(2).asc))
+ }
+
+ test("group by ordinal") {
+ // Tests group by ordinal, apply single rule.
+ val plan2 = testRelation2.groupBy(Literal(1), Literal(2))('a, 'b)
+ comparePlans(
+ new SubstituteUnresolvedOrdinals(conf).apply(plan2),
+ testRelation2.groupBy(UnresolvedOrdinal(1), UnresolvedOrdinal(2))('a, 'b))
+
+ // Tests group by ordinal, do full analysis
+ checkAnalysis(plan2, testRelation2.groupBy(a, b)(a, b))
+
+ // group by ordinal can be turned off by config
+ comparePlans(
+ new SubstituteUnresolvedOrdinals(conf.copy(groupByOrdinal = false)).apply(plan2),
+ testRelation2.groupBy(Literal(1), Literal(2))('a, 'b))
+ }
+}
http://git-wip-us.apache.org/repos/asf/spark/blob/3e6ef2e8/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitutionSuite.scala
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitutionSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitutionSuite.scala
deleted file mode 100644
index 23995e9..0000000
--- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/UnresolvedOrdinalSubstitutionSuite.scala
+++ /dev/null
@@ -1,65 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.spark.sql.catalyst.analysis
-
-import org.apache.spark.sql.catalyst.analysis.TestRelations.testRelation2
-import org.apache.spark.sql.catalyst.dsl.expressions._
-import org.apache.spark.sql.catalyst.dsl.plans._
-import org.apache.spark.sql.catalyst.expressions.Literal
-import org.apache.spark.sql.catalyst.SimpleCatalystConf
-
-class UnresolvedOrdinalSubstitutionSuite extends AnalysisTest {
-
- test("test rule UnresolvedOrdinalSubstitution, replaces ordinal in order by or group by") {
- val a = testRelation2.output(0)
- val b = testRelation2.output(1)
- val conf = new SimpleCatalystConf(caseSensitiveAnalysis = true)
-
- // Expression OrderByOrdinal is unresolved.
- assert(!UnresolvedOrdinal(0).resolved)
-
- // Tests order by ordinal, apply single rule.
- val plan = testRelation2.orderBy(Literal(1).asc, Literal(2).asc)
- comparePlans(
- new UnresolvedOrdinalSubstitution(conf).apply(plan),
- testRelation2.orderBy(UnresolvedOrdinal(1).asc, UnresolvedOrdinal(2).asc))
-
- // Tests order by ordinal, do full analysis
- checkAnalysis(plan, testRelation2.orderBy(a.asc, b.asc))
-
- // order by ordinal can be turned off by config
- comparePlans(
- new UnresolvedOrdinalSubstitution(conf.copy(orderByOrdinal = false)).apply(plan),
- testRelation2.orderBy(Literal(1).asc, Literal(2).asc))
-
-
- // Tests group by ordinal, apply single rule.
- val plan2 = testRelation2.groupBy(Literal(1), Literal(2))('a, 'b)
- comparePlans(
- new UnresolvedOrdinalSubstitution(conf).apply(plan2),
- testRelation2.groupBy(UnresolvedOrdinal(1), UnresolvedOrdinal(2))('a, 'b))
-
- // Tests group by ordinal, do full analysis
- checkAnalysis(plan2, testRelation2.groupBy(a, b)(a, b))
-
- // group by ordinal can be turned off by config
- comparePlans(
- new UnresolvedOrdinalSubstitution(conf.copy(groupByOrdinal = false)).apply(plan2),
- testRelation2.groupBy(Literal(1), Literal(2))('a, 'b))
- }
-}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org