Skip to content

Commit

Permalink
Add a check for empty IN list to IN Presto predicate (facebookincubat…
Browse files Browse the repository at this point in the history
…or#7514)

Summary:
A version of IN predicate that takes non-constant IN list was missing a check for empty list.

Fixes facebookincubator#7507

Pull Request resolved: facebookincubator#7514

Reviewed By: xiaoxmeng

Differential Revision: D51199918

Pulled By: mbasmanova

fbshipit-source-id: d6ea9c24bcf3a24c9cc8909cb7dc9af71277fc7a
  • Loading branch information
mbasmanova authored and facebook-github-bot committed Nov 10, 2023
1 parent b44b855 commit ae0c450
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 4 deletions.
4 changes: 3 additions & 1 deletion velox/functions/prestosql/InPredicate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class GenericInPredicate : public exec::VectorFunction {
result->clearNulls(rows);
auto* boolResult = result->asUnchecked<FlatVector<bool>>();

rows.applyToSelected([&](vector_size_t row) {
context.applyToSelectedNoThrow(rows, [&](vector_size_t row) {
if (value->isNullAt(row) || inList->isNullAt(row)) {
boolResult->setNull(row, true);
return;
Expand All @@ -57,6 +57,8 @@ class GenericInPredicate : public exec::VectorFunction {
const auto offset = inListBaseArray->offsetAt(arrayRow);
const auto size = inListBaseArray->sizeAt(arrayRow);

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

bool hasNull = false;
for (auto i = 0; i < size; ++i) {
if (inListElements->containsNullAt(offset + i)) {
Expand Down
22 changes: 19 additions & 3 deletions velox/functions/prestosql/tests/InPredicateTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "velox/common/base/tests/GTestUtils.h"
#include "velox/functions/prestosql/tests/utils/FunctionBaseTest.h"

using namespace facebook::velox::test;
Expand Down Expand Up @@ -938,12 +939,12 @@ TEST_F(InPredicateTest, nonConstantInList) {

auto expected = makeNullableFlatVector<bool>({
true, // 1 in [1, 2, 3]
false, // 2 in []
std::nullopt, // 2 in [] -> error or null if under try
false, // 3 in [1, 2]
std::nullopt, // 4 in null
true, // 5 in [1, 2, null, 3, 4, 5]
std::nullopt, // 6 in [null, 2, null, 3]
std::nullopt, // null in null
std::nullopt, // null in []]
false, // 8 in [1, 3, 5, 7, 9, 11]
});

Expand All @@ -955,8 +956,23 @@ TEST_F(InPredicateTest, nonConstantInList) {
},
"in");

auto result = evaluate(in, data);
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(1, false);
rows.updateBounds();

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

} // namespace
Expand Down

0 comments on commit ae0c450

Please sign in to comment.