From b35c240da7ef4bc86ace780e2676aaef5e8b8ab4 Mon Sep 17 00:00:00 2001 From: seebs Date: Wed, 1 Mar 2023 23:49:45 -0600 Subject: [PATCH] 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 --- sql3/planner/expression.go | 2 +- sql3/planner/expressionagg.go | 3 + sql3/planner/oppqlgroupby.go | 18 +++++- sql3/planner/planoptimizer.go | 2 +- sql3/test/defs/defs_having.go | 102 +++++++++++++++++++++++++++++++++- 5 files changed, 120 insertions(+), 7 deletions(-) diff --git a/sql3/planner/expression.go b/sql3/planner/expression.go index 33937e6e0..3e585910c 100644 --- a/sql3/planner/expression.go +++ b/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 diff --git a/sql3/planner/expressionagg.go b/sql3/planner/expressionagg.go index e1098c1f8..eab7f098d 100644 --- a/sql3/planner/expressionagg.go +++ b/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 } diff --git a/sql3/planner/oppqlgroupby.go b/sql3/planner/oppqlgroupby.go index 5687a9e73..fb0d4387e 100644 --- a/sql3/planner/oppqlgroupby.go +++ b/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) diff --git a/sql3/planner/planoptimizer.go b/sql3/planner/planoptimizer.go index 114c29922..039930582 100644 --- a/sql3/planner/planoptimizer.go +++ b/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) { diff --git a/sql3/test/defs/defs_having.go b/sql3/test/defs/defs_having.go index a9aa2abec..3b282ce52 100644 --- a/sql3/test/defs/defs_having.go +++ b/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,20 +20,20 @@ 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)), @@ -39,5 +41,99 @@ var selectHavingTests = TableTest{ 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, + // }, }, }