Skip to content

Commit 7115014

Browse files
Kevin Wilfongfacebook-github-bot
Kevin Wilfong
authored andcommitted
find_first and find_first_index have default null behavior (facebookincubator#10598)
Summary: Pull Request resolved: facebookincubator#10598 I verified in Presto Java find_first and find_first_index have default null behavior. In Velox they do not which leads to inconsistent behavior when actually evaluating the function could produce errors that are skipped thanks to the optimizations that can be done when default null behavior is set. The original code that set the non-default behavior included the following comment // find_first function is null preserving for the array argument, but // predicate expression may use other fields and may not preserve nulls in // these. // For example: find_first(array[1, 2, 3], x -> x > coalesce(a, 0)) should // not return null when 'a' is null. If I'm reading this right, it's saying that null elements in the array may not produce null results in the lambda. AFAIK default null behavior isn't recursive so it only matters if the entire array is null. There are already tests that verify if the array is NULL the result is NULL, I added tests that verify if the optional start index argument is NULL the result is NULL. I verified these passed before and after my change, so this does not change the behavior of the function except in the presence of errors. This is to address facebookincubator#10564 Reviewed By: pedroerp Differential Revision: D60412963 fbshipit-source-id: 4f890fddc88e82363b27028f8e0d4a46f4c48107
1 parent d5d672c commit 7115014

File tree

2 files changed

+11
-4
lines changed

2 files changed

+11
-4
lines changed

velox/functions/prestosql/FindFirst.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -403,16 +403,14 @@ std::vector<std::shared_ptr<exec::FunctionSignature>> indexSignatures() {
403403
/// For example: find_first(array[1, 2, 3], x -> x > coalesce(a, 0)) should
404404
/// not return null when 'a' is null.
405405

406-
VELOX_DECLARE_VECTOR_FUNCTION_WITH_METADATA(
406+
VELOX_DECLARE_VECTOR_FUNCTION(
407407
udf_find_first,
408408
valueSignatures(),
409-
exec::VectorFunctionMetadataBuilder().defaultNullBehavior(false).build(),
410409
std::make_unique<FindFirstFunction>());
411410

412-
VELOX_DECLARE_VECTOR_FUNCTION_WITH_METADATA(
411+
VELOX_DECLARE_VECTOR_FUNCTION(
413412
udf_find_first_index,
414413
indexSignatures(),
415-
exec::VectorFunctionMetadataBuilder().defaultNullBehavior(false).build(),
416414
std::make_unique<FindFirstIndexFunction>());
417415

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

velox/functions/prestosql/tests/FindFirstTest.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,15 @@ TEST_F(FindFirstTest, basic) {
101101
expected =
102102
makeNullableFlatVector<int64_t>({3, std::nullopt, std::nullopt, 5});
103103
verify("find_first_index(c0, -2, x -> (x > 0))", data, expected);
104+
105+
expected = makeNullConstant(TypeKind::INTEGER, 4);
106+
verify("find_first(c0, cast(null as INTEGER), x -> (x > 0))", data, expected);
107+
108+
expected = makeNullConstant(TypeKind::BIGINT, 4);
109+
verify(
110+
"find_first_index(c0, cast(null as INTEGER), x -> (x > 0))",
111+
data,
112+
expected);
104113
}
105114

106115
TEST_F(FindFirstTest, firstMatchIsNull) {

0 commit comments

Comments
 (0)