From 34eb3a46efe497c34f417b1701e6ef722d57f077 Mon Sep 17 00:00:00 2001
From: Michael Armbrust <michael@databricks.com>
Date: Thu, 12 Feb 2015 00:01:11 -0800
Subject: [PATCH] more work

---
 .../spark/sql/catalyst/analysis/Analyzer.scala      | 13 ++++++-------
 .../sql/catalyst/expressions/namedExpressions.scala |  2 +-
 .../spark/sql/catalyst/analysis/AnalysisSuite.scala | 11 +++++++++++
 .../scala/org/apache/spark/sql/SQLQuerySuite.scala  |  6 ++----
 4 files changed, 20 insertions(+), 12 deletions(-)

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 df2f10de68463..105282c22aa7a 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
@@ -83,7 +83,7 @@ class Analyzer(catalog: Catalog,
           operator transformAllExpressions {
             case a: Attribute if !a.resolved =>
               val from = operator.inputSet.map(_.name).mkString("{", ", ", "}")
-              failAnalysis(s"cannot resolve '$a' given input columns $from")
+              failAnalysis(s"cannot resolve '${a.prettyString}' given input columns $from")
 
             case c: Cast if !c.resolved =>
               failAnalysis(
@@ -93,13 +93,13 @@ class Analyzer(catalog: Catalog,
               failAnalysis(
                 s"invalid expression ${b.prettyString} " +
                 s"between ${b.left.simpleString} and ${b.right.simpleString}")
-
-
           }
 
           operator match {
             case f: Filter if f.condition.dataType != BooleanType =>
-              failAnalysis(s"filter expression '${f.condition.prettyString}' is not a boolean.")
+              failAnalysis(
+                s"filter expression '${f.condition.prettyString}' " +
+                s"of type ${f.condition.dataType.simpleString} is not a boolean.")
 
             case aggregatePlan @ Aggregate(groupingExprs, aggregateExprs, child) =>
               def isValidAggregateExpression(expr: Expression): Boolean = expr match {
@@ -118,11 +118,10 @@ class Analyzer(catalog: Catalog,
                   case Alias(g: GetField, _) => g
                 })
               }.foreach { e =>
-                failAnalysis(s"expression must be aggregates or be in group by $e")
+                failAnalysis(
+                  s"expression '${e.prettyString}' is not an aggregate function or in the group by")
               }
 
-              aggregatePlan
-
             case o if o.children.nonEmpty && !o.references.subsetOf(o.inputSet) =>
               val missingAttributes = (o.references -- o.inputSet).map(_.prettyString).mkString(",")
               val input = o.inputSet.map(_.prettyString).mkString(",")
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala
index f77c56311cc8c..62c062be6d820 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala
@@ -218,7 +218,7 @@ case class PrettyAttribute(name: String) extends Attribute with trees.LeafNode[E
   override def exprId: ExprId = ???
   override def eval(input: Row): EvaluatedType = ???
   override def nullable: Boolean = ???
-  override def dataType: DataType = ???
+  override def dataType: DataType = NullType
 }
 
 object VirtualColumn {
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
index 99083d35ad235..e70c651e1486e 100644
--- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
+++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
@@ -136,6 +136,17 @@ class AnalysisSuite extends FunSuite with BeforeAndAfter {
     testRelation.select(Literal(1).cast(BinaryType).as('badCast)),
     "invalid cast" :: Literal(1).dataType.simpleString :: BinaryType.simpleString :: Nil)
 
+  errorTest(
+    "non-boolean filters",
+    testRelation.where(Literal(1)),
+    "filter" :: "'1'" :: "not a boolean" :: Literal(1).dataType.simpleString :: Nil)
+
+  errorTest(
+    "missing group by",
+    testRelation2.groupBy('a)('b),
+    "'b'" :: "group by" :: Nil
+  )
+
   case class UnresolvedTestPlan() extends LeafNode {
     override lazy val resolved = false
     override def output = Nil
diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
index bba8899651259..a1c8cf58f2357 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
@@ -806,10 +806,8 @@ class SQLQuerySuite extends QueryTest with BeforeAndAfterAll {
   test("throw errors for non-aggregate attributes with aggregation") {
     def checkAggregation(query: String, isInvalidQuery: Boolean = true) {
       if (isInvalidQuery) {
-        val e = intercept[TreeNodeException[LogicalPlan]](sql(query).queryExecution.analyzed)
-        assert(
-          e.getMessage.startsWith("Expression not in GROUP BY"),
-          "Non-aggregate attribute(s) not detected\n")
+        val e = intercept[AnalysisException](sql(query).queryExecution.analyzed)
+        assert(e.getMessage contains "group by")
       } else {
         // Should not throw
         sql(query).queryExecution.analyzed