diff --git a/dax/queryer/orchestrator.go b/dax/queryer/orchestrator.go index 0ed57a9c1..c77684125 100644 --- a/dax/queryer/orchestrator.go +++ b/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, 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. diff --git a/dax/test/dax/dax_test.go b/dax/test/dax/dax_test.go index d4e3f7d9d..c1cda5636 100644 --- a/dax/test/dax/dax_test.go +++ b/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 diff --git a/executor.go b/executor.go index 5b804c4c2..2f59d9436 100644 --- a/executor.go +++ b/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, 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. diff --git a/executor_test.go b/executor_test.go index 7c61f6f82..309f31fa5 100644 --- a/executor_test.go +++ b/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 diff --git a/field.go b/field.go index 302e380df..34763ec79 100644 --- a/field.go +++ b/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") diff --git a/sql3/test/defs/defs_aggregate.go b/sql3/test/defs/defs_aggregate.go index 0532d3c40..ea43ec730 100644 --- a/sql3/test/defs/defs_aggregate.go +++ b/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, - //}, }, }