Skip to content

Commit

Permalink
fix: Classification functions should return [] instead null
Browse files Browse the repository at this point in the history
Summary:
To be consistent with Java behavior,  values that may produce non-sensible results should return [] instead of NULL just like in Java.

See failed fuzzer
```
10 of extra rows:
	43958 | null | 1.614066005835391E308 | null | 56 | null
	43958 | null | 1.249509044542672E308 | null | 73 | null
	43958 | null | 1.0352077108850509E308 | null | 133 | null
	43958 | null | 1.4197168072885737E308 | null | 222 | null
	43958 | null | 1.5812716007918045E308 | null | 251 | null
	43958 | null | 1.2735686950279541E308 | null | 256 | null
	43958 | null | 3.911438594915475E307 | null | 257 | null
	43958 | null | 1.2969345942976502E307 | null | 258 | null
	43958 | null | 1.2608837131771955E308 | null | 273 | null
	43958 | null | 1.7284583913651944E308 | null | 275 | null

10 of missing rows:
	43958 | null | 1.614066005835391E308 | null | 56 | []
	43958 | null | 1.249509044542672E308 | null | 73 | []
	43958 | null | 1.0352077108850509E308 | null | 133 | []
	43958 | null | 1.4197168072885737E308 | null | 222 | []
	43958 | null | 1.5812716007918045E308 | null | 251 | []
	43958 | null | 1.2735686950279541E308 | null | 256 | []
	43958 | null | 3.911438594915475E307 | null | 257 | []
	43958 | null | 1.2969345942976502E307 | null | 258 | []
	43958 | null | 1.2608837131771955E308 | null | 273 | []
	43958 | null | 1.7284583913651944E308 | null | 275 | []
```

This was discovered as a task to me after the original diff landed; however, the original diff had green signal on the window fuzzer.

Differential Revision: D67246837
  • Loading branch information
yuandagits authored and facebook-github-bot committed Dec 15, 2024
1 parent 12942c1 commit 645f70c
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ class ClassificationAggregation : public exec::Aggregate {
auto* accumulator = value<Accumulator<type>>(group);
const auto size = accumulator->size();
if (isNull(group)) {
vector->setNull(i, true);
clearNull(rawNulls, i);
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,18 @@ TEST_F(ClassificationAggregationTest, basic) {
runTest("classification_miss_rate(5, c0, c1)", input, expected);
runTest("classification_thresholds(5, c0, c1)", input, expected);

/// Test case when output should be [].
input = makeRowVector({
makeNullableFlatVector<bool>({true}),
makeNullableFlatVector<double>({std::nullopt}),
});
expected = makeRowVector({makeArrayVector<double>({{}})});
runTest("classification_fall_out(5, c0, c1)", input, expected);
runTest("classification_precision(5, c0, c1)", input, expected);
runTest("classification_recall(5, c0, c1)", input, expected);
runTest("classification_miss_rate(5, c0, c1)", input, expected);
runTest("classification_thresholds(5, c0, c1)", input, expected);

/// Test invalid bucket count test
input = makeRowVector({
makeNullableFlatVector<bool>({true}),
Expand Down

0 comments on commit 645f70c

Please sign in to comment.