Skip to content

Commit

Permalink
Throw if Filter clause has non boolean expression
Browse files Browse the repository at this point in the history
Summary:
There is difference in behavior in how non boolean aggregation masks behave in Velox and Presto. 
This change causes velox to throw when we encounter non boolean mask.
This is a stop gap for issue facebookincubator#11523.

Differential Revision: D65858866
  • Loading branch information
Krishna Pai authored and facebook-github-bot committed Nov 13, 2024
1 parent 05dc841 commit 07f92c9
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 0 deletions.
1 change: 1 addition & 0 deletions velox/exec/AggregationMasks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ void AggregationMasks::addInput(

// Get the projection column vector that would be our mask.
const auto& maskVector = input->childAt(entry.first);
VELOX_CHECK_EQ(maskVector->type(), BOOLEAN(), "Mask must be BOOLEAN");

// Get decoded vector and update the masked selectivity vector.
decodedMask_.decode(*maskVector, rows);
Expand Down
34 changes: 34 additions & 0 deletions velox/exec/tests/AggregationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,40 @@ TEST_F(AggregationTest, groupingSetsEmptyInput) {
}));
}

TEST_F(AggregationTest, disableNonBooleanMasks) {
auto data = makeRowVector(
{"c0", "c1"},
{makeFlatVector<int64_t>({1, -1, 0, -2, 10}),
makeFlatVector<std::string>({"a", "a", "b", "c", "a"})});

auto plan = PlanBuilder()
.values({data})
.aggregation(
{"c1"},
{"count(c0) FILTER(WHERE c0)"},
{},
core::AggregationNode::Step::kPartial,
false)
.planNode();

VELOX_ASSERT_THROW(
AssertQueryBuilder(plan).copyResults(pool()), "Mask must be BOOLEAN");

// Planbuilder doesnt allow expressions in FILTER clauses
plan = PlanBuilder()
.values({data})
.project({"c0", "c1", "c0 > 0 as mask"})
.aggregation(
{"c1"},
{"count(c0) FILTER(WHERE mask)"},
{},
core::AggregationNode::Step::kPartial,
true)
.planNode();

AssertQueryBuilder(plan).copyResults(pool());
}

TEST_F(AggregationTest, outputBatchSizeCheckWithSpill) {
const int numVectors = 5;
const int vectorSize = 20;
Expand Down

0 comments on commit 07f92c9

Please sign in to comment.