[SPARK-8075][SQL] apply type check interface to more expressions #6723

cloud-fan · 2015-06-09T15:11:42Z

a follow up of #6405.
Note: It's not a big change, a lot of changing is due to I swap some code in aggregates.scala to make aggregate functions right below its corresponding aggregate expressions.

SparkQA · 2015-06-09T15:19:36Z

Test build #34506 has finished for PR 6723 at commit 64d78a8.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

cloud-fan · 2015-06-09T15:35:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

   */
-  def checkInputDataTypes(): TypeCheckResult = TypeCheckResult.TypeCheckSuccess
+  def checkInputDataTypes(): TypeCheckResult


Do we really want this? There are a lot of expressions that don't need type check...

ok i agree with you maybe we should implement this by default to be always "success", so we don't need to define this for every expression.

cloud-fan · 2015-06-09T15:42:07Z

cc @rxin , according to here and here, do we require BinaryExpression to have 2 equal-type children? If so, I think we should add java doc of BinaryExpression to say that, or we should create a subclass of BinaryExpression which has equal-type children.

SparkQA · 2015-06-09T17:19:43Z

Test build #34509 has finished for PR 6723 at commit 305be74.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

SparkQA · 2015-06-09T17:42:35Z

Test build #34508 has finished for PR 6723 at commit 6f09d05.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

rxin · 2015-06-09T23:45:43Z

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

@@ -89,6 +89,8 @@ case class UnresolvedFunction(name: String, children: Seq[Expression]) extends E
    throw new TreeNodeException(this, s"No function to evaluate expression. type: ${this.nodeName}")

  override def toString: String = s"'$name(${children.mkString(",")})"
+
+  override def checkInputDataTypes(): TypeCheckResult = TypeCheckResult.TypeCheckSuccess


why do they need to be here?

cloud-fan · 2015-06-11T09:47:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/windowExpressions.scala

@@ -69,7 +69,7 @@ case class WindowSpecDefinition(
  override def children: Seq[Expression] = partitionSpec ++ orderSpec

  override lazy val resolved: Boolean =
-    childrenResolved && frameSpecification.isInstanceOf[SpecifiedWindowFrame]
+    childrenResolved && checkInputDataTypes().isSuccess && frameSpecification.isInstanceOf[SpecifiedWindowFrame]


I'm thinking of making resolved final, and add a overridable method like extraResolveLogic, so that people won't forget to check types when they override resolved.

SparkQA · 2015-06-11T09:51:43Z

Test build #34684 has finished for PR 6723 at commit 3c3c87b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

SparkQA · 2015-06-19T09:07:24Z

Test build #35244 has finished for PR 6723 at commit 0faa15a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2015-06-21T09:19:21Z

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

+        findTightestCommonTypeAndPromoteToString(types) match {
+          case Some(finalDataType) => CreateArray(children.map(Cast(_, finalDataType)))
+          case None => a
+        }


I checked with hive, CreateArray should cast input to string, but with some rules like Coalesce did. For example we should not cast boolean to string for CreateArray.

SparkQA · 2015-06-21T09:56:16Z

Test build #35387 has finished for PR 6723 at commit 343074b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

SparkQA · 2015-06-21T10:45:38Z

Test build #35388 has finished for PR 6723 at commit 2811da0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class Module(object):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

SparkQA · 2015-06-21T15:54:59Z

Test build #35392 has finished for PR 6723 at commit bb9948a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class Module(object):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

SparkQA · 2015-06-24T06:54:24Z

Test build #35637 has finished for PR 6723 at commit 2124301.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
- case class CountDistinctFunction(
- case class ApproxCountDistinctPartitionFunction(
- case class ApproxCountDistinctMergeFunction(
- case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class CombineSum(child: Expression) extends AggregateExpression
- case class SumDistinct(child: Expression)
- case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
- case class CombineSetsAndSumFunction(
- case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
- case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]

cloud-fan · 2015-06-24T08:16:00Z

cc @rxin

marmbrus · 2015-06-24T23:26:23Z

Thanks! Merging to master.

cloud-fan force-pushed the type-check branch 2 times, most recently from 6f09d05 to 305be74 Compare June 9, 2015 15:32

cloud-fan reviewed Jun 9, 2015
View reviewed changes

rxin reviewed Jun 9, 2015
View reviewed changes

cloud-fan force-pushed the type-check branch 2 times, most recently from 8423bc7 to 3c3c87b Compare June 11, 2015 09:43

cloud-fan reviewed Jun 11, 2015
View reviewed changes

cloud-fan force-pushed the type-check branch 2 times, most recently from 772866d to 0faa15a Compare June 19, 2015 07:14

cloud-fan force-pushed the type-check branch from 0faa15a to 343074b Compare June 21, 2015 09:14

cloud-fan reviewed Jun 21, 2015
View reviewed changes

cloud-fan force-pushed the type-check branch from 343074b to 8efce5f Compare June 21, 2015 09:28

cloud-fan force-pushed the type-check branch from 8efce5f to 2811da0 Compare June 21, 2015 09:32

cloud-fan added 3 commits June 24, 2015 12:46

apply type check interface to more expressions

287d3bb

add tests

5a658bb

fix tests

2124301

cloud-fan force-pushed the type-check branch from bb9948a to 2124301 Compare June 24, 2015 04:50

asfgit closed this in b71d325 Jun 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8075][SQL] apply type check interface to more expressions #6723

[SPARK-8075][SQL] apply type check interface to more expressions #6723

cloud-fan commented Jun 9, 2015

SparkQA commented Jun 9, 2015

cloud-fan Jun 9, 2015

rxin Jun 10, 2015

cloud-fan commented Jun 9, 2015

SparkQA commented Jun 9, 2015

SparkQA commented Jun 9, 2015

rxin Jun 9, 2015

cloud-fan Jun 11, 2015

SparkQA commented Jun 11, 2015

SparkQA commented Jun 19, 2015

cloud-fan Jun 21, 2015

SparkQA commented Jun 21, 2015

SparkQA commented Jun 21, 2015

SparkQA commented Jun 21, 2015

SparkQA commented Jun 24, 2015

cloud-fan commented Jun 24, 2015

marmbrus commented Jun 24, 2015

[SPARK-8075][SQL] apply type check interface to more expressions #6723

[SPARK-8075][SQL] apply type check interface to more expressions #6723

Conversation

cloud-fan commented Jun 9, 2015

SparkQA commented Jun 9, 2015

cloud-fan Jun 9, 2015

Choose a reason for hiding this comment

rxin Jun 10, 2015

Choose a reason for hiding this comment

cloud-fan commented Jun 9, 2015

SparkQA commented Jun 9, 2015

SparkQA commented Jun 9, 2015

rxin Jun 9, 2015

Choose a reason for hiding this comment

cloud-fan Jun 11, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 11, 2015

SparkQA commented Jun 19, 2015

cloud-fan Jun 21, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 21, 2015

SparkQA commented Jun 21, 2015

SparkQA commented Jun 21, 2015

SparkQA commented Jun 24, 2015

cloud-fan commented Jun 24, 2015

marmbrus commented Jun 24, 2015