Skip to content

Commit

Permalink
Correctly error for mixed units where one is an empty unit string
Browse files Browse the repository at this point in the history
  • Loading branch information
suyashkumar committed Aug 13, 2024
1 parent fce4610 commit 738226b
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
13 changes: 8 additions & 5 deletions interpreter/operator_aggregate.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,19 +310,22 @@ func (i *interpreter) evalMedianQuantity(_ model.IUnaryExpression, operand resul

values := make([]float64, 0, len(l))
var unit model.Unit
for _, elem := range l {
for idx, elem := range l {
if result.IsNull(elem) {
continue
}
v, err := result.ToQuantity(elem)
if err != nil {
return result.Value{}, err
}
// We only support List<Quantity> where all the elements have the exact same unit, since we do not support
// mixed unit Quantity math in our engine yet.
if unit == "" {
// We only support List<Quantity> where all the elements have the exact same unit, since we
// do not support mixed unit Quantity math in our engine yet.
if idx == 0 {
unit = v.Unit
} else if unit != v.Unit {
}
if unit != v.Unit {
// TODO: b/342061715 - technically we should treat '' unit and '1' unit as the same, but
// for now we don't (and we should apply this globally).
return result.Value{}, fmt.Errorf("Median(List<Quantity>) operand has different units which is not supported, got %v and %v", unit, v.Unit)
}
values = append(values, v.Value)
Expand Down
7 changes: 6 additions & 1 deletion tests/enginetests/operator_aggregate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,11 @@ func TestMedian_Error(t *testing.T) {
cql: "Median({1 'cm', 2 'g'})",
wantErrContains: "Median(List<Quantity>) operand has different units which is not supported",
},
{
name: "Median({1 '', 2 'g'})",
cql: "Median({1 '', 2 'g'})",
wantErrContains: "Median(List<Quantity>) operand has different units which is not supported",
},
}

for _, tc := range tests {
Expand All @@ -694,7 +699,7 @@ func TestMedian_Error(t *testing.T) {
}

_, err = interpreter.Eval(context.Background(), parsedLibs, defaultInterpreterConfig(t, p))
if !strings.Contains(err.Error(), tc.wantErrContains) {
if err == nil || !strings.Contains(err.Error(), tc.wantErrContains) {
t.Errorf("Eval returned unexpected error: %v, want error containing %q", err, tc.wantErrContains)
}
})
Expand Down

0 comments on commit 738226b

Please sign in to comment.