handle count(*) in having correctly (#2274)

* correct reference for `having count(*)`

It turns out that `having count(*) ...` was always treating
the count(*) as exactly 1. After studying this a lot, I noticed
that in fact, we correctly handle other counts. The reason is
that there's already code to recognize aggregates in `having`
clauses as matching aggregates that are being computed -- but
it only covers the other aggregate clause types, not the newly
added `countStarPlanExpression` from making `count(*)` work even
if there's no `_id` field.

We add several corresponding test cases.

* fix sum(a_decimal) type conversion

Added a test case for this, and also added a fix for it.
Underlying issue: qualifiedRefPlanExpression could end up
producing an int64 instead of a pql.Decimal, even though it
had expected type Decimal.

Originally this worked by politely converting an int64 to
a pql.Decimal in the Evaluate phase, but this was not ideal;
the real question is why it was coming out as an int64 at
that step. Showed this to Pat, who spent a while studying it
and produced a better fix.

* temporarily comment out test which fails in DAX

Author: seebs

sql3/planner/expression.go

@@ -538,7 +538,7 @@ func (n *binOpPlanExpression) Evaluate(currentRow []interface{}) (interface{}, e
 				return nil, sql3.NewErrInternalf("unhandled operator %d", n.op)
 			}
 		}
-		return nil, sql3.NewErrInternalf("unexpected type conversion error '%t', '%t'", nlok, nrok)
+		return nil, sql3.NewErrInternalf("unexpected type conversion error '%T', '%T'", coercedLhs, coercedRhs)
 
 	case *parser.DataTypeTimestamp:
 		//if either side is nil, return nil

sql3/planner/expressionagg.go

@@ -122,6 +122,9 @@ func (n *countStarPlanExpression) Plan() map[string]interface{} {
 	result["_expr"] = fmt.Sprintf("%T", n)
 	result["description"] = n.String()
 	result["dataType"] = n.Type().TypeDescription()
+	if n.arg != nil {
+		result["arg"] = n.arg.Plan()
+	}
 	return result
 }
 

sql3/planner/oppqlgroupby.go

@@ -244,13 +244,27 @@ func (i *pqlGroupByRowIter) Next(ctx context.Context) (types.Row, error) {
 		}
 		//now populate the aggregate value
 		aggIdx := len(i.groupByColumns)
-		switch i.aggregate.(type) {
+		switch agg := i.aggregate.(type) {
 		case *countPlanExpression, *countStarPlanExpression:
 			row[aggIdx] = int64(group.Count)
 
-		case *countDistinctPlanExpression, *sumPlanExpression:
+		case *countDistinctPlanExpression:
 			row[aggIdx] = int64(group.Agg)
 
+		case *sumPlanExpression:
+			switch ty := agg.Type().(type) {
+			case *parser.DataTypeDecimal:
+				if group.DecimalAgg == nil {
+					row[aggIdx] = pql.NewDecimal(int64(group.Agg), ty.Scale)
+				} else {
+					row[aggIdx] = *group.DecimalAgg
+				}
+			case *parser.DataTypeInt:
+				row[aggIdx] = int64(group.Agg)
+			default:
+				return nil, sql3.NewErrInternalf("unhandled sum return type '%T'", ty)
+			}
+
 		case *avgPlanExpression:
 			if group.DecimalAgg == nil {
 				average := float64(group.Agg) / float64(group.Count)

sql3/planner/planoptimizer.go

@@ -1355,7 +1355,7 @@ func fixFieldRefIndexesForHaving(ctx context.Context, scope *OptimizerScope, a *
 	return TransformExpr(exp, func(e types.PlanExpression) (types.PlanExpression, bool, error) {
 		switch typedExpr := e.(type) {
 		case *sumPlanExpression, *countPlanExpression, *countDistinctPlanExpression,
-			*avgPlanExpression, *minPlanExpression, *maxPlanExpression,
+			*avgPlanExpression, *minPlanExpression, *maxPlanExpression, *countStarPlanExpression,
 			*percentilePlanExpression:
 			for i, col := range schema {
 				if strings.EqualFold(typedExpr.String(), col.ColumnName) {

sql3/test/defs/defs_having.go

@@ -1,5 +1,7 @@
 package defs
 
+import "github.com/featurebasedb/featurebase/v3/pql"
+
 var selectHavingTests = TableTest{
 	name: "select-having",
 	Table: tbl(
@@ -18,26 +20,120 @@ var selectHavingTests = TableTest{
 			srcRow(int64(2), int64(22), []int64{21, 22, 23}, int64(201), "str2", []string{"a2", "b2", "c2"}, float64(234.56)),
 			srcRow(int64(3), int64(33), []int64{31, 32, 33}, int64(301), "str3", []string{"a3", "b3", "c3"}, float64(345.67)),
 			srcRow(int64(4), int64(44), []int64{41, 42, 43}, int64(401), "str4", []string{"a4", "b4", "c4"}, float64(456.78)),
+			srcRow(int64(5), int64(11), []int64{11, 12, 13}, int64(101), "str1", []string{"a5", "b5", "c5"}, float64(567.89)),
 		),
 	),
 	SQLTests: []SQLTest{
 		{
-			name: "select-having",
+			name: "countfieldincluded",
 			SQLs: sqls(
-				"select count(*), an_int from having_test group by an_int having count(*) > 0",
+				"select count(an_int), an_int from having_test group by an_int having count(an_int) = 1",
 			),
 			ExpHdrs: hdrs(
 				hdr("", fldTypeInt),
 				hdr("an_int", fldTypeInt),
 			),
 			ExpRows: rows(
-				row(int64(1), int64(11)),
 				row(int64(1), int64(22)),
 				row(int64(1), int64(33)),
 				row(int64(1), int64(44)),
 			),
 			Compare:        CompareExactUnordered,
 			SortStringKeys: true,
 		},
+		{
+			name: "countfieldnotincluded",
+			SQLs: sqls(
+				"select an_int from having_test group by an_int having count(an_int) = 1",
+			),
+			ExpHdrs: hdrs(
+				hdr("an_int", fldTypeInt),
+			),
+			ExpRows: rows(
+				row(int64(22)),
+				row(int64(33)),
+				row(int64(44)),
+			),
+			Compare:        CompareExactUnordered,
+			SortStringKeys: true,
+		},
+		{
+			name: "countstarincluded",
+			SQLs: sqls(
+				"select count(*), an_int from having_test group by an_int having count(*) > 1",
+			),
+			ExpHdrs: hdrs(
+				hdr("", fldTypeInt),
+				hdr("an_int", fldTypeInt),
+			),
+			ExpRows: rows(
+				row(int64(2), int64(11)),
+			),
+			Compare:        CompareExactUnordered,
+			SortStringKeys: true,
+		},
+		{
+			name: "countstarnotincluded",
+			SQLs: sqls(
+				"select an_int from having_test group by an_int having count(*) > 1",
+			),
+			ExpHdrs: hdrs(
+				hdr("an_int", fldTypeInt),
+			),
+			ExpRows: rows(
+				row(
+					int64(11),
+				),
+			),
+			Compare:        CompareExactUnordered,
+			SortStringKeys: true,
+		},
+		{
+			name: "sum-dec",
+			SQLs: sqls(
+				"select sum(a_decimal), an_int from having_test group by an_int having sum(a_decimal) < 250.00",
+			),
+			ExpHdrs: hdrs(
+				hdr("", fldTypeDecimal2),
+				hdr("an_int", fldTypeInt),
+			),
+			ExpRows: rows(
+				row(pql.NewDecimal(23456, 2), int64(22)),
+			),
+			Compare:        CompareExactUnordered,
+			SortStringKeys: true,
+		},
+		{
+			name: "sum-int",
+			SQLs: sqls(
+				"select sum(an_int), an_int from having_test group by an_int having sum(an_int) < 25",
+			),
+			ExpHdrs: hdrs(
+				hdr("", fldTypeInt),
+				hdr("an_int", fldTypeInt),
+			),
+			ExpRows: rows(
+				row(int64(22), int64(11)),
+				row(int64(22), int64(22)),
+			),
+			Compare:        CompareExactUnordered,
+			SortStringKeys: true,
+		},
+		// Fails in DAX because the string isn't translated.
+		// {
+		// 	name: "string",
+		// 	SQLs: sqls(
+		// 		"select a_string, count(*) from having_test group by a_string having count(*) > 1",
+		// 	),
+		// 	ExpHdrs: hdrs(
+		// 		hdr("a_string", fldTypeString),
+		// 		hdr("", fldTypeInt),
+		// 	),
+		// 	ExpRows: rows(
+		// 		row(string("str1"), int64(2)),
+		// 	),
+		// 	Compare:        CompareExactUnordered,
+		// 	SortStringKeys: true,
+		// },
 	},
 }