make percentile work on Decimals, also make Percentile slightly better

So there's a lot going on here.

Percentile just did not work, even a little, with decimals.

In theory we try to make the int val part of ValCount work, in
ValCountize, but you can't actually use that for everything because
it unconditionally adds bsig.Base even when it shouldn't. But it
doesn't matter that we were returning those values from, say,
(Field).MinForShard, because ValCount.Smaller was not preserving them
when identifying the smaller of two Decimal ValCounts anyway.
And even if it did, the logic in Percentile wouldn't have worked
with passing the raw unscaled integer in as a value to compare
against.

But that's fine because the logic was also more generally wrong.
According to the existing logic, a value is the median value if
exactly as many values are less than it as are greater than it.

This is... not actually very accurate to what we usually mean by
"median". Because some values are *equal* to a given value. So
for instance, say you have the values {1, 1, 1, [a million 2s], 3}.
Our logic would regard 2 as being too high to be the median, because
3 times as many values are lower as are higher.

New interpretation: Imagine a sorted list of all your values, with
N entries. You want the Nth percentile, which is to say, you want N%
of values to be less than the vale you pick, and (100-N)% to be greater.
You can round both of these down. So for instance, if you have 6 values,
and want the median, you want 3 values greater, and 3 values less. To
be picky, we could demand the average of those middle two values, but
we're not in a good position to do that in this implementation.

If the number of desired things less than, or greater than, a target
is 0, we can short-circuit to the minimum or maximum value. This can
happen when nth is close to an end and the number of things is small,
not just at nth=0/nth=100.

So we rework this, and we rework the tests for this behavior to reflect
that logic.

We change executePercentile to be able to return a nil rather than
a weird ValCount in cases where there's no result, such as when
there's no values to compute a percentile of.

We also change the SQL tests to match the new behavior, since some
of them were expecting everything done on a decimal field with values
10-13 to come back as 10.00 as a decimal because that is what the
code returned.

We also propagate these changes to DAX, and along the way, fix up a
TODO item in the DAX copy, and stop skipping the test that was
failing because of that TODO item.

dax/queryer/orchestrator.go

@@ -894,145 +894,297 @@ func (o *orchestrator) executeMax(ctx context.Context, tableKeyer dax.TableKeyer
 	return other, nil
 }
 
-// TODO(jaffee) fix this... valcountize assumes access to field details like base
-// executePercentile executes a Percentile() call.
-func (o *orchestrator) executePercentile(ctx context.Context, tableKeyer dax.TableKeyer, c *pql.Call, shards []uint64, opt *featurebase.ExecOptions) (_ featurebase.ValCount, err error) {
+// executePercentile executes a Percentile() call. This logic is mirrored from
+// featurebase executor, but we should probably replace it with a smarter algorithm.
+func (o *orchestrator) executePercentile(ctx context.Context, tableKeyer dax.TableKeyer, c *pql.Call, shards []uint64, opt *featurebase.ExecOptions) (_ interface{}, err error) {
 	span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executePercentile")
 	defer span.Finish()
 
 	// get nth
 	var nthFloat float64
-	nthArg, ok := c.Args["nth"]
-	if !ok {
-		return featurebase.ValCount{}, errors.New(errors.ErrUncoded, "Percentile(): nth required")
-	}
+	nthArg := c.Args["nth"]
 	switch nthArg := nthArg.(type) {
 	case pql.Decimal:
 		nthFloat = nthArg.Float64()
 	case int64:
 		nthFloat = float64(nthArg)
+	case nil:
+		return nil, errors.New(errors.ErrUncoded, "Percentile(): nth required")
 	default:
-		return featurebase.ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be a number between 0 and 100 inclusive", c.Args["nth"])
+		return nil, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be a number between 0 and 100 inclusive", c.Args["nth"])
 	}
 	if nthFloat < 0 || nthFloat > 100.0 {
-		return featurebase.ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be a number between 0 and 100 inclusive", nthFloat)
+		return nil, errors.Errorf("Percentile(): invalid nth value (%f), should be a number between 0 and 100 inclusive", nthFloat)
 	}
 
 	// get field
 	fieldName, err := c.FirstStringArg("field", "_field")
 	if err != nil {
-		return featurebase.ValCount{}, errors.New(errors.ErrUncoded, "Percentile(): field required")
+		return nil, errors.New(errors.ErrUncoded, "Percentile(): field required")
 	}
 	field, err := o.schemaFieldInfo(ctx, tableKeyer, fieldName)
 	if err != nil {
-		return featurebase.ValCount{}, ErrFieldNotFound
+		return nil, ErrFieldNotFound
 	}
 
 	// filter call for min & max
 	var filterCall *pql.Call
 
+	// We want to know the total number of values, so that when we check
+	// for values <X, or >X, we are also able to infer the number of values
+	// equal to X.
+	var totalCountCall *pql.Call
 	// check if filter provided
 	if filterArg, ok := c.Args["filter"].(*pql.Call); ok && filterArg != nil {
+		// You could supply a filter like `Not(x=3)` which would yield values
+		// which exist in the database but are null in this field, we don't
+		// want that.
 		filterCall = filterArg
+		totalCountCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name: "Intersect",
+					Children: []*pql.Call{
+						filterCall,
+						{
+							Name: "Row",
+							Args: map[string]interface{}{
+								fieldName: &pql.Condition{
+									Op:    pql.NEQ,
+									Value: nil,
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+	} else {
+		// request a count of IS NOT NULL, aka Row(field!=null). We care about
+		// the actual number of results that should exist.
+		totalCountCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name: "Row",
+					Args: map[string]interface{}{
+						fieldName: &pql.Condition{
+							Op:    pql.NEQ,
+							Value: nil,
+						},
+					},
+				},
+			},
+		}
+	}
+	// total values matched by the filter (if it exists) or that aren't null
+	totalCountInterface, err := o.executeCall(ctx, tableKeyer, totalCountCall, shards, opt)
+	totalCount, ok := totalCountInterface.(uint64)
+	if !ok || totalCount == 0 {
+		// it's not an error, but the median of nothing is NULL.
+		return nil, nil
 	}
 
+	// We have totalCount values. If nth is 50, we want half the values to be
+	// above us, and half below us. So for instance, if we have 6 values, we want
+	// 3 above us, and 3 below us. For odd numbers, we can round these *both*
+	// down -- for 7 values, we'd want 3 higher, and 3 lower.
+	desiredLess := uint64((float64(totalCount) * nthFloat) / 100.0)
+	desiredGreater := uint64((float64(totalCount) * (100 - nthFloat)) / 100.0)
+
 	// get min
-	q, _ := pql.ParseString(fmt.Sprintf(`Min(field="%s")`, fieldName))
-	minCall := q.Calls[0]
-	if filterCall != nil {
-		minCall.Children = append(minCall.Children, filterCall)
-	}
-	minVal, err := o.executeMin(ctx, tableKeyer, minCall, shards, opt)
-	if err != nil {
-		return featurebase.ValCount{}, errors.Wrap(err, "executing Min call for Percentile")
-	}
-	if nthFloat == 0.0 {
-		return minVal, nil
+	var minVal featurebase.ValCount
+
+	if desiredGreater != 0 {
+		q, err := pql.ParseString(fmt.Sprintf(`Min(field="%s")`, fieldName))
+		if err != nil {
+			return nil, errors.Wrap(err, "parsing max call for Percentile")
+		}
+		minCall := q.Calls[0]
+		if filterCall != nil {
+			minCall.Children = append(minCall.Children, filterCall)
+		}
+		minVal, err = o.executeMin(ctx, tableKeyer, minCall, shards, opt)
+		if err != nil {
+			return nil, errors.Wrap(err, "executing Min call for Percentile")
+		}
+
+		if desiredLess == 0 {
+			if minVal.DecimalVal != nil {
+				minVal.FloatVal = minVal.DecimalVal.Float64()
+			}
+			return minVal, nil
+		}
 	}
 
 	// get max
-	q, _ = pql.ParseString(fmt.Sprintf(`Max(field="%s")`, fieldName))
+	q, err := pql.ParseString(fmt.Sprintf(`Max(field="%s")`, fieldName))
+	if err != nil {
+		return nil, errors.Wrap(err, "parsing max call for Percentile")
+	}
 	maxCall := q.Calls[0]
 	if filterCall != nil {
 		maxCall.Children = append(maxCall.Children, filterCall)
 	}
 	maxVal, err := o.executeMax(ctx, tableKeyer, maxCall, shards, opt)
 	if err != nil {
-		return featurebase.ValCount{}, errors.Wrap(err, "executing Max call for Percentile")
+		return nil, errors.Wrap(err, "executing Max call for Percentile")
 	}
-	// set up reusables
-	var countCall, rangeCall *pql.Call
-	if filterCall == nil {
-		countQuery, _ := pql.ParseString(fmt.Sprintf("Count(Row(%s < 0))", fieldName))
-		countCall = countQuery.Calls[0]
-		rangeCall = countCall.Children[0]
+
+	if desiredGreater == 0 {
+		if maxVal.DecimalVal != nil {
+			maxVal.FloatVal = maxVal.DecimalVal.Float64()
+		}
+		return maxVal, nil
+	}
+
+	// o.executeCount(ctx, tableKeyer, countCall, shards, opt)
+	// cookValCount(possibleNthVal, 1, field), nil
+	// the logic here is basically identical whether we're doing a decimal field
+	// or an integer field, but the actual code used to compare maximum and minimum
+	// values, or extract values from valCount objects, differs.
+	// So we set up generic functions which will produce the right values.
+	var averageMinMax func() interface{}
+	var minLessthanMax func() bool
+	var maxValueUnder func(interface{})
+	var minValueOver func(interface{})
+
+	if field.Options.Type == FieldTypeDecimal {
+		minPtr := minVal.DecimalVal
+		maxPtr := maxVal.DecimalVal
+		if minPtr == nil {
+			return nil, fmt.Errorf("unexpectedly nil min value in percentile")
+		}
+		if maxPtr == nil {
+			return nil, fmt.Errorf("unexpectedly nil max value in percentile")
+		}
+		min := *minPtr
+		max := *maxPtr
+		two := pql.NewDecimal(2, 0)
+		one := pql.NewDecimal(1, field.Options.Scale)
+		averageMinMax = func() interface{} {
+			return pql.DivideDecimal(pql.AddDecimal(min, max), two)
+		}
+		minLessthanMax = func() bool {
+			return min.LessThan(max)
+		}
+		maxValueUnder = func(v interface{}) {
+			max = pql.SubtractDecimal(v.(pql.Decimal), one)
+		}
+		minValueOver = func(v interface{}) {
+			min = pql.AddDecimal(v.(pql.Decimal), one)
+		}
 	} else {
-		countQuery, _ := pql.ParseString(fmt.Sprintf(`Count(Intersect(Row(%s < 0)))`, fieldName))
-		countCall = countQuery.Calls[0]
-		intersectCall := countCall.Children[0]
-		intersectCall.Children = append(intersectCall.Children, filterCall)
-		rangeCall = intersectCall.Children[0]
+		// plain BSI field
+		min := minVal.Val
+		max := maxVal.Val
+		averageMinMax = func() interface{} {
+			// min+max could overflow, in theory, but if they're both odd, we want one
+			// higher than min/2 + max/2.
+			return (min / 2) + (max / 2) + (((min % 2) + (max % 2)) / 2)
+		}
+		minLessthanMax = func() bool {
+			return min < max
+		}
+		maxValueUnder = func(v interface{}) {
+			max = v.(int64) - 1
+		}
+		minValueOver = func(v interface{}) {
+			min = v.(int64) + 1
+		}
 	}
 
-	k := (100 - nthFloat) / nthFloat
+	// set up reusable pql.Call objects representing a count (or intersectioncount,
+	// if we have a filter) with a condition we can alter.
+	var countCall, rangeCall *pql.Call
+	rangeCondition := pql.Condition{
+		Op:    pql.LT,
+		Value: nil,
+	}
+	rangeCall = &pql.Call{
+		Name: "Row",
+		Args: map[string]interface{}{
+			fieldName: &rangeCondition,
+		},
+	}
+	if filterCall == nil {
+		countCall = &pql.Call{
+			Name:     "Count",
+			Children: []*pql.Call{rangeCall},
+		}
+	} else {
+		countCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name:     "Intersect",
+					Children: []*pql.Call{rangeCall, filterCall},
+				},
+			},
+		}
+	}
 
-	min, max := minVal.Val, maxVal.Val
 	// estimate nth val, eg median when nth=0.5
-	for min < max {
+	// we start with a blind guess of minVal, so if min and max are equal,
+	// we just fall out of the loop. If they're not, we compute the middle value
+	// of whatever range we're looking at, and compare it to our expectations of
+	// how many
+	var possibleNthVal interface{}
+	if minVal.DecimalVal != nil {
+		possibleNthVal = minVal.DecimalVal
+	} else {
+		possibleNthVal = minVal.Val
+	}
+	for minLessthanMax() {
 		// compute average without integer overflow, then correct for division of
 		// odd numbers by 2
-		possibleNthVal := ((max / 2) + (min / 2)) + (((max % 2) + (min % 2)) / 2)
-		// possibleNthVal = (max + min) / 2
-		// get left count
-		rangeCall.Args[fieldName] = &pql.Condition{
-			Op:    pql.Token(pql.LT),
-			Value: possibleNthVal,
-		}
-		leftCountUint64, err := o.executeCount(ctx, tableKeyer, countCall, shards, opt)
+		possibleNthVal = averageMinMax()
+		rangeCondition.Value = possibleNthVal
+		rangeCondition.Op = pql.LT
+		leftCount, err := o.executeCount(ctx, tableKeyer, countCall, shards, opt)
 		if err != nil {
-			return featurebase.ValCount{}, errors.Wrap(err, "executing Count call L for Percentile")
+			return nil, errors.Wrap(err, "executing Count call L for Percentile")
 		}
-		leftCount := int64(leftCountUint64)
 
-		// get right count
-		rangeCall.Args[fieldName] = &pql.Condition{
-			Op:    pql.Token(pql.GT),
-			Value: possibleNthVal,
+		// If there's more things less than possibleNthVal than our desired number
+		// of things less, we need to look at the left side of this.
+		if leftCount > desiredLess {
+			maxValueUnder(possibleNthVal)
+			continue
 		}
-		rightCountUint64, err := o.executeCount(ctx, tableKeyer, countCall, shards, opt)
+
+		rangeCondition.Op = pql.GT
+		rightCount, err := o.executeCount(ctx, tableKeyer, countCall, shards, opt)
 		if err != nil {
-			return featurebase.ValCount{}, errors.Wrap(err, "executing Count call R for Percentile")
+			return nil, errors.Wrap(err, "executing Count call R for Percentile")
 		}
-		rightCount := int64(rightCountUint64)
-
-		// 'weight' the left count as per k
-		leftCountWeighted := int64(math.Round(k * float64(leftCount)))
-
-		// binary search
-		if leftCountWeighted > rightCount {
-			max = possibleNthVal - 1
-		} else if leftCountWeighted < rightCount {
-			min = possibleNthVal + 1
-		} else {
-			return cookValCount(possibleNthVal, 1, field), nil
+		// If there's more things greater than the desired number, we need to look to the right.
+		if rightCount > desiredGreater {
+			minValueOver(possibleNthVal)
+			continue
 		}
-	}
 
-	return cookValCount(min, 1, field), nil
-}
-
-func cookValCount(val int64, cnt uint64, field *featurebase.FieldInfo) featurebase.ValCount {
-	valCount := featurebase.ValCount{Count: int64(cnt)}
-	base := field.Options.Base
-	switch field.Options.Type {
-	case featurebase.FieldTypeDecimal:
-		dec := pql.NewDecimal(val+base, field.Options.Scale)
-		valCount.DecimalVal = &dec
-	case FieldTypeTimestamp:
-		valCount.TimestampVal = time.Unix(0, (val+base)*featurebase.TimeUnitNanos(field.Options.TimeUnit)).UTC()
+		// min and max may be different, but the number of values above and below this
+		// value are both reasonable. For instance, with 7 items and looking for median,
+		// we'd have 3 less and 3 greater, and we can't really do better than that.
+		break
+	}
+	switch v := possibleNthVal.(type) {
+	case int64:
+		return featurebase.ValCount{
+			Val:   v,
+			Count: 1,
+		}, nil
+	case pql.Decimal:
+		return featurebase.ValCount{
+			DecimalVal: &v,
+			FloatVal:   v.Float64(),
+			Count:      1,
+		}, nil
+	default:
+		return nil, fmt.Errorf("unexpected percentile Nth value type %T", possibleNthVal)
 	}
-	valCount.Val = val + base
-	return valCount
 }
 
 // executeMinRow executes a MinRow() call.

dax/test/dax/dax_test.go

@@ -142,7 +142,6 @@ func TestDAXIntegration(t *testing.T) {
 		// need to get these passing before alpha.
 		skips := []string{
 			"testinsert/test-5",             // error messages differ
-			"percentile_test/test-6",        // related to TODO in orchestrator.executePercentile
 			"alterTable/alterTableBadTable", // looks like table does not exist is a different error in DAX
 			"top-limit-tests/test-2",        // don't know why this is failing at all
 			"top-limit-tests/test-3",        // don't know why this is failing at all

executor.go

@@ -1294,129 +1294,310 @@ func (e *executor) executeMax(ctx context.Context, qcx *Qcx, index string, c *pq
 }
 
 // executePercentile executes a Percentile() call.
-func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shards []uint64, opt *ExecOptions) (_ ValCount, err error) {
+//
+// To compute the percentile, we find the maximum and minimum values that match
+// our filter (or an implicit filter of "value isn't null"), and also count values
+// matching our filter. We convert our percentile to an approximate number of
+// values that should be higher or lower than the desired value. If either of those
+// is zero, we return the minimum/maximum; otherwise, we do a binary search of
+// the range between minimum and maximum, looking for a value which has the
+// desired number of values higher or lower than it.
+//
+// Unfortunately, each step in this process is its own, separate, cluster-wide
+// query. this should be replaced with a modern probabilistic algorithm, which
+// could accumulate statistical information per shard and combine that information
+// in a single pass.
+func (e *executor) executePercentile(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shards []uint64, opt *ExecOptions) (result interface{}, err error) {
+	// defer func() {
+	// 	fmt.Fprintf(os.Stderr, "executePercentile %s: %#v, %v\n",
+	// 		c.String(), result, err)
+	// }()
 	span, ctx := tracing.StartSpanFromContext(ctx, "executor.executePercentile")
 	defer span.Finish()
 
 	// get nth
 	var nthFloat float64
-	nthArg, ok := c.Args["nth"]
-	if !ok {
-		return ValCount{}, errors.New("Percentile(): nth required")
-	}
+	nthArg := c.Args["nth"]
 	switch nthArg := nthArg.(type) {
 	case pql.Decimal:
 		nthFloat = nthArg.Float64()
 	case int64:
 		nthFloat = float64(nthArg)
+	case nil:
+		return nil, errors.New("Percentile(): nth required")
 	default:
-		return ValCount{}, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be a number between 0 and 100 inclusive", c.Args["nth"])
+		return nil, errors.Errorf("Percentile(): invalid nth='%v' of type (%[1]T), should be a number between 0 and 100 inclusive", c.Args["nth"])
 	}
 	if nthFloat < 0 || nthFloat > 100.0 {
-		return ValCount{}, errors.Errorf("Percentile(): invalid nth value (%f), should be a number between 0 and 100 inclusive", nthFloat)
+		return nil, errors.Errorf("Percentile(): invalid nth value (%f), should be a number between 0 and 100 inclusive", nthFloat)
 	}
 
 	// get field
 	fieldName, err := c.FirstStringArg("field", "_field")
 	if err != nil {
-		return ValCount{}, errors.New("Percentile(): field required")
+		return nil, errors.New("Percentile(): field required")
 	}
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return ValCount{}, ErrFieldNotFound
+		return nil, ErrFieldNotFound
 	}
 
 	// filter call for min & max
 	var filterCall *pql.Call
 
+	// We want to know the total number of values, so that when we check
+	// for values <X, or >X, we are also able to infer the number of values
+	// equal to X.
+	var totalCountCall *pql.Call
 	// check if filter provided
 	if filterArg, ok := c.Args["filter"].(*pql.Call); ok && filterArg != nil {
+		// You could supply a filter like `Not(x=3)` which would yield values
+		// which exist in the database but are null in this field, we don't
+		// want that.
 		filterCall = filterArg
+		totalCountCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name: "Intersect",
+					Children: []*pql.Call{
+						filterCall,
+						{
+							Name: "Row",
+							Args: map[string]interface{}{
+								fieldName: &pql.Condition{
+									Op:    pql.NEQ,
+									Value: nil,
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+	} else {
+		// request a count of IS NOT NULL, aka Row(field!=null). We care about
+		// the actual number of results that should exist.
+		totalCountCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name: "Row",
+					Args: map[string]interface{}{
+						fieldName: &pql.Condition{
+							Op:    pql.NEQ,
+							Value: nil,
+						},
+					},
+				},
+			},
+		}
+	}
+	// total values matched by the filter (if it exists) or that aren't null
+	totalCountInterface, err := e.executeCall(ctx, qcx, index, totalCountCall, shards, opt)
+	totalCount, ok := totalCountInterface.(uint64)
+	if !ok || totalCount == 0 {
+		// it's not an error, but the median of nothing is NULL.
+		return nil, nil
 	}
 
+	// We have totalCount values. If nth is 50, we want half the values to be
+	// above us, and half below us. So for instance, if we have 6 values, we want
+	// 3 above us, and 3 below us. For odd numbers, we can round these *both*
+	// down -- for 7 values, we'd want 3 higher, and 3 lower.
+	desiredLess := uint64((float64(totalCount) * nthFloat) / 100.0)
+	desiredGreater := uint64((float64(totalCount) * (100 - nthFloat)) / 100.0)
+
 	// get min
-	q, _ := pql.ParseString(fmt.Sprintf(`Min(field="%s")`, fieldName))
-	minCall := q.Calls[0]
-	if filterCall != nil {
-		minCall.Children = append(minCall.Children, filterCall)
-	}
-	minVal, err := e.executeMin(ctx, qcx, index, minCall, shards, opt)
-	if err != nil {
-		return ValCount{}, errors.Wrap(err, "executing Min call for Percentile")
-	}
-	if nthFloat == 0.0 {
-		return minVal, nil
+	var minVal ValCount
+
+	if desiredGreater != 0 {
+		q, err := pql.ParseString(fmt.Sprintf(`Min(field="%s")`, fieldName))
+		if err != nil {
+			return nil, errors.Wrap(err, "parsing max call for Percentile")
+		}
+		minCall := q.Calls[0]
+		if filterCall != nil {
+			minCall.Children = append(minCall.Children, filterCall)
+		}
+		minVal, err = e.executeMin(ctx, qcx, index, minCall, shards, opt)
+		if err != nil {
+			return nil, errors.Wrap(err, "executing Min call for Percentile")
+		}
+
+		if desiredLess == 0 {
+			if minVal.DecimalVal != nil {
+				minVal.FloatVal = minVal.DecimalVal.Float64()
+			}
+			return minVal, nil
+		}
 	}
 
 	// get max
-	q, _ = pql.ParseString(fmt.Sprintf(`Max(field="%s")`, fieldName))
+	q, err := pql.ParseString(fmt.Sprintf(`Max(field="%s")`, fieldName))
+	if err != nil {
+		return nil, errors.Wrap(err, "parsing max call for Percentile")
+	}
 	maxCall := q.Calls[0]
 	if filterCall != nil {
 		maxCall.Children = append(maxCall.Children, filterCall)
 	}
 	maxVal, err := e.executeMax(ctx, qcx, index, maxCall, shards, opt)
 	if err != nil {
-		return ValCount{}, errors.Wrap(err, "executing Max call for Percentile")
+		return nil, errors.Wrap(err, "executing Max call for Percentile")
 	}
-	// set up reusables
-	var countCall, rangeCall *pql.Call
-	if filterCall == nil {
-		countQuery, _ := pql.ParseString(fmt.Sprintf("Count(Row(%s < 0))", fieldName))
-		countCall = countQuery.Calls[0]
-		rangeCall = countCall.Children[0]
+
+	if desiredGreater == 0 {
+		if maxVal.DecimalVal != nil {
+			maxVal.FloatVal = maxVal.DecimalVal.Float64()
+		}
+		return maxVal, nil
+	}
+
+	// the logic here is basically identical whether we're doing a decimal field
+	// or an integer field, but the actual code used to compare maximum and minimum
+	// values, or extract values from valCount objects, differs.
+	// So we set up generic functions which will produce the right values.
+	var averageMinMax func() interface{}
+	var minLessthanMax func() bool
+	var maxValueUnder func(interface{})
+	var minValueOver func(interface{})
+
+	if field.options.Type == FieldTypeDecimal {
+		minPtr := minVal.DecimalVal
+		maxPtr := maxVal.DecimalVal
+		if minPtr == nil {
+			return nil, fmt.Errorf("unexpectedly nil min value in percentile")
+		}
+		if maxPtr == nil {
+			return nil, fmt.Errorf("unexpectedly nil max value in percentile")
+		}
+		min := *minPtr
+		max := *maxPtr
+		two := pql.NewDecimal(2, 0)
+		one := pql.NewDecimal(1, field.options.Scale)
+		averageMinMax = func() interface{} {
+			return pql.DivideDecimal(pql.AddDecimal(min, max), two)
+		}
+		minLessthanMax = func() bool {
+			return min.LessThan(max)
+		}
+		maxValueUnder = func(v interface{}) {
+			max = pql.SubtractDecimal(v.(pql.Decimal), one)
+		}
+		minValueOver = func(v interface{}) {
+			min = pql.AddDecimal(v.(pql.Decimal), one)
+		}
 	} else {
-		countQuery, _ := pql.ParseString(fmt.Sprintf(`Count(Intersect(Row(%s < 0)))`, fieldName))
-		countCall = countQuery.Calls[0]
-		intersectCall := countCall.Children[0]
-		intersectCall.Children = append(intersectCall.Children, filterCall)
-		rangeCall = intersectCall.Children[0]
+		// plain BSI field
+		min := minVal.Val
+		max := maxVal.Val
+		averageMinMax = func() interface{} {
+			// min+max could overflow, in theory, but if they're both odd, we want one
+			// higher than min/2 + max/2.
+			return (min / 2) + (max / 2) + (((min % 2) + (max % 2)) / 2)
+		}
+		minLessthanMax = func() bool {
+			return min < max
+		}
+		maxValueUnder = func(v interface{}) {
+			max = v.(int64) - 1
+		}
+		minValueOver = func(v interface{}) {
+			min = v.(int64) + 1
+		}
 	}
 
-	k := (100 - nthFloat) / nthFloat
+	// set up reusable pql.Call objects representing a count (or intersectioncount,
+	// if we have a filter) with a condition we can alter.
+	var countCall, rangeCall *pql.Call
+	rangeCondition := pql.Condition{
+		Op:    pql.LT,
+		Value: nil,
+	}
+	rangeCall = &pql.Call{
+		Name: "Row",
+		Args: map[string]interface{}{
+			fieldName: &rangeCondition,
+		},
+	}
+	if filterCall == nil {
+		countCall = &pql.Call{
+			Name:     "Count",
+			Children: []*pql.Call{rangeCall},
+		}
+	} else {
+		countCall = &pql.Call{
+			Name: "Count",
+			Children: []*pql.Call{
+				{
+					Name:     "Intersect",
+					Children: []*pql.Call{rangeCall, filterCall},
+				},
+			},
+		}
+	}
 
-	min, max := minVal.Val, maxVal.Val
 	// estimate nth val, eg median when nth=0.5
-	for min < max {
+	// we start with a blind guess of minVal, so if min and max are equal,
+	// we just fall out of the loop. If they're not, we compute the middle value
+	// of whatever range we're looking at, and compare it to our expectations of
+	// how many
+	var possibleNthVal interface{}
+	if minVal.DecimalVal != nil {
+		possibleNthVal = minVal.DecimalVal
+	} else {
+		possibleNthVal = minVal.Val
+	}
+	for minLessthanMax() {
 		// compute average without integer overflow, then correct for division of
 		// odd numbers by 2
-		possibleNthVal := ((max / 2) + (min / 2)) + (((max % 2) + (min % 2)) / 2)
-		// possibleNthVal = (max + min) / 2
-		// get left count
-		rangeCall.Args[fieldName] = &pql.Condition{
-			Op:    pql.Token(pql.LT),
-			Value: possibleNthVal,
-		}
-		leftCountUint64, err := e.executeCount(ctx, qcx, index, countCall, shards, opt)
+		possibleNthVal = averageMinMax()
+		rangeCondition.Value = possibleNthVal
+		rangeCondition.Op = pql.LT
+		leftCount, err := e.executeCount(ctx, qcx, index, countCall, shards, opt)
 		if err != nil {
-			return ValCount{}, errors.Wrap(err, "executing Count call L for Percentile")
+			return nil, errors.Wrap(err, "executing Count call L for Percentile")
 		}
-		leftCount := int64(leftCountUint64)
 
-		// get right count
-		rangeCall.Args[fieldName] = &pql.Condition{
-			Op:    pql.Token(pql.GT),
-			Value: possibleNthVal,
+		// If there's more things less than possibleNthVal than our desired number
+		// of things less, we need to look at the left side of this.
+		if leftCount > desiredLess {
+			maxValueUnder(possibleNthVal)
+			continue
 		}
-		rightCountUint64, err := e.executeCount(ctx, qcx, index, countCall, shards, opt)
+
+		rangeCondition.Op = pql.GT
+		rightCount, err := e.executeCount(ctx, qcx, index, countCall, shards, opt)
 		if err != nil {
-			return ValCount{}, errors.Wrap(err, "executing Count call R for Percentile")
+			return nil, errors.Wrap(err, "executing Count call R for Percentile")
 		}
-		rightCount := int64(rightCountUint64)
-
-		// 'weight' the left count as per k
-		leftCountWeighted := int64(math.Round(k * float64(leftCount)))
-
-		// binary search
-		if leftCountWeighted > rightCount {
-			max = possibleNthVal - 1
-		} else if leftCountWeighted < rightCount {
-			min = possibleNthVal + 1
-		} else {
-			return field.valCountize(possibleNthVal, 1, nil)
+		// If there's more things greater than the desired number, we need to look to the right.
+		if rightCount > desiredGreater {
+			minValueOver(possibleNthVal)
+			continue
 		}
+
+		// min and max may be different, but the number of values above and below this
+		// value are both reasonable. For instance, with 7 items and looking for median,
+		// we'd have 3 less and 3 greater, and we can't really do better than that.
+		break
+	}
+	switch v := possibleNthVal.(type) {
+	case int64:
+		return ValCount{
+			Val:   v,
+			Count: 1,
+		}, nil
+	case pql.Decimal:
+		return ValCount{
+			DecimalVal: &v,
+			FloatVal:   v.Float64(),
+			Count:      1,
+		}, nil
+	default:
+		return nil, fmt.Errorf("unexpected percentile Nth value type %T", possibleNthVal)
 	}
-
-	return field.valCountize(min, 1, nil)
 }
 
 // executeMinRow executes a MinRow() call.

executor_test.go

@@ -7641,13 +7641,22 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) {
 		if nth == 0.0 {
 			return min
 		}
-		k := (100 - nth) / nth
-
+		if nth == 100.0 {
+			return max
+		}
 		possibleNthVal := int64(0)
+		desiredLess := int((float64(len(nums)) * nth) / 100.0)
+		desiredGreater := int((float64(len(nums)) * (100 - nth)) / 100.0)
+		if desiredLess == 0 {
+			return min
+		}
+		if desiredGreater == 0 {
+			return max
+		}
 		// bin search
 		for min < max {
 			possibleNthVal = ((max / 2) + (min / 2)) + (((max % 2) + (min % 2)) / 2)
-			leftCount, rightCount := int64(0), int64(0)
+			leftCount, rightCount := 0, 0
 			for _, num := range nums {
 				if num < possibleNthVal {
 					leftCount++
@@ -7656,11 +7665,9 @@ func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) {
 				}
 			}
 
-			leftCountWeighted := int64(math.Round(k * float64(leftCount)))
-
-			if leftCountWeighted > rightCount {
+			if leftCount > desiredLess {
 				max = possibleNthVal - 1
-			} else if leftCountWeighted < rightCount {
+			} else if rightCount > desiredGreater {
 				min = possibleNthVal + 1
 			} else { // perfectly balanced, as all things should be
 				return possibleNthVal

field.go

@@ -1627,6 +1627,11 @@ func (f *Field) MinForShard(qcx *Qcx, shard uint64, filter *Row) (ValCount, erro
 // includes the int64 "Val\" value to make comparisons easier in the
 // executor (at time of writing, Percentile takes advantage of this,
 // but we might be able to simplify logic in other places as well).
+//
+// Note that the ValCount returned has bsig.Base included, or if
+// you specify a nil bsig, includes the field's bsig.Base. Which is
+// to say, don't use this if you have a value that's already been
+// adjusted by base.
 func (f *Field) valCountize(val int64, cnt uint64, bsig *bsiGroup) (ValCount, error) {
 	if bsig == nil {
 		bsig = f.bsiGroup(f.name)
@@ -1637,10 +1642,10 @@ func (f *Field) valCountize(val int64, cnt uint64, bsig *bsiGroup) (ValCount, er
 	}
 	valCount := ValCount{Count: int64(cnt)}
 
-	if f.Options().Type == FieldTypeDecimal {
+	if f.options.Type == FieldTypeDecimal {
 		dec := pql.NewDecimal(val+bsig.Base, bsig.Scale)
 		valCount.DecimalVal = &dec
-	} else if f.Options().Type == FieldTypeTimestamp {
+	} else if f.options.Type == FieldTypeTimestamp {
 		ts, err := ValToTimestamp(f.options.TimeUnit, val+bsig.Base)
 		if err != nil {
 			return ValCount{}, errors.Wrap(err, "translating value to timestamp")

sql3/test/defs/defs_aggregate.go

@@ -502,7 +502,7 @@ var percentileTests = TableTest{
 				hdr("p_rows", fldTypeInt),
 			),
 			ExpRows: rows(
-				row(int64(12)),
+				row(int64(11)),
 			),
 			Compare: CompareExactUnordered,
 		},
@@ -514,9 +514,7 @@ var percentileTests = TableTest{
 				hdr("p_rows", fldTypeDecimal2),
 			),
 			ExpRows: rows(
-				// This should probably be (1200, 2), not (1000, 2).
-				// TODO: look into this when investigating the percentile/WHERE bug.
-				row(pql.NewDecimal(1000, 2)),
+				row(pql.NewDecimal(1150, 2)),
 			),
 			Compare: CompareExactUnordered,
 		},
@@ -528,24 +526,22 @@ var percentileTests = TableTest{
 				hdr("p_rows", fldTypeInt),
 			),
 			ExpRows: rows(
-				row(int64(12)),
+				row(int64(11)),
+			),
+			Compare: CompareExactUnordered,
+		},
+		{
+			SQLs: sqls(
+				"SELECT percentile(d1, 50) AS p_rows FROM percentile_test WHERE d1 < 13",
+			),
+			ExpHdrs: hdrs(
+				hdr("p_rows", fldTypeDecimal2),
+			),
+			ExpRows: rows(
+				row(pql.NewDecimal(1100, 2)),
 			),
 			Compare: CompareExactUnordered,
 		},
-		// This test is failing! It seems to be returning the count of elements < 13,
-		// rather than processing them for percentile.
-		//{
-		//	SQLs: sqls(
-		//		"SELECT percentile(d1, 50) AS p_rows FROM percentile_test WHERE d1 < 13",
-		//	),
-		//	ExpHdrs: hdrs(
-		//		hdr("p_rows", fldTypeDecimal2),
-		//	),
-		//	ExpRows: rows(
-		//		row(pql.NewDecimal(1200, 2)),
-		//	),
-		//	Compare: CompareExactUnordered,
-		//},
 	},
 }