Skip to content

Commit

Permalink
Fix generic in-predicate for complex types (facebookincubator#7545)
Browse files Browse the repository at this point in the history
Summary:
This fixes a bug where the generic in-predicate, which is employed
when the in-list is not a constant, can return null if the input
contains a null even though the in-list is empty. The correct
behavior is that it should throw an error for the empty in-list.

Fixes facebookincubator#7533

Pull Request resolved: facebookincubator#7545

Test Plan: Added unit test

Reviewed By: mbasmanova

Differential Revision: D51277276

Pulled By: bikramSingh91

fbshipit-source-id: 65373617327da0a3432dbc22c5c7bf4fd34ab2b8
  • Loading branch information
bikramSingh91 authored and facebook-github-bot committed Nov 15, 2023
1 parent 31111e7 commit 1c5387d
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 6 deletions.
12 changes: 6 additions & 6 deletions velox/functions/prestosql/InPredicate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ class GenericInPredicate : public exec::VectorFunction {
return;
}

const auto valueBaseRow = value->index(row);
if (valueBase->containsNullAt(valueBaseRow)) {
boolResult->setNull(row, true);
return;
}

const auto arrayRow = inList->index(row);
const auto offset = inListBaseArray->offsetAt(arrayRow);
const auto size = inListBaseArray->sizeAt(arrayRow);

VELOX_USER_CHECK_GT(size, 0, "IN list must not be empty");

const auto valueBaseRow = value->index(row);
if (valueBase->containsNullAt(valueBaseRow)) {
boolResult->setNull(row, true);
return;
}

bool hasNull = false;
for (auto i = 0; i < size; ++i) {
if (inListElements->containsNullAt(offset + i)) {
Expand Down
51 changes: 51 additions & 0 deletions velox/functions/prestosql/tests/InPredicateTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -975,5 +975,56 @@ TEST_F(InPredicateTest, nonConstantInList) {
assertEqualVectors(expected, result, rows);
}

TEST_F(InPredicateTest, nonConstantComplexInList) {
auto data = makeRowVector({
makeArrayVectorFromJson<int32_t>({
"null",
"[1, null, 3]",
"[1, null, 3]",
"[1, 2, 3]",
}),
makeArrayVector(
{0, 1, 1, 1},
makeArrayVectorFromJson<int32_t>({
"[1, 2, 3]",
"[1, 2, 3]",
}),
{1}),
});

auto expected = makeNullableFlatVector<bool>({
std::nullopt, // Input is null
std::nullopt, // in-list is null
std::nullopt, // in-list is empty
true,
});

auto in = std::make_shared<core::CallTypedExpr>(
BOOLEAN(),
std::vector<core::TypedExprPtr>{
field(ARRAY(INTEGER()), "c0"),
field(ARRAY(ARRAY(INTEGER())), "c1"),
},
"in");

auto tryIn = std::make_shared<core::CallTypedExpr>(
BOOLEAN(), std::vector<core::TypedExprPtr>{in}, "try");

// Evaluate "in" on all rows. Expect an error.
VELOX_ASSERT_THROW(evaluate(in, data), "IN list must not be empty");

// Evaluate "try(in)" on all rows.
auto result = evaluate(tryIn, data);
assertEqualVectors(expected, result);

// Evaluate "in" on a subset of rows that should not generate an error.
SelectivityVector rows(data->size());
rows.setValid(2, false);
rows.updateBounds();

result = evaluate(in, data, rows);
assertEqualVectors(expected, result, rows);
}

} // namespace
} // namespace facebook::velox::functions

0 comments on commit 1c5387d

Please sign in to comment.