Add any_match and none_match Presto functions #4327

duanmeng · 2023-03-17T12:35:00Z

any_match(array(T), function(T, boolean)) → boolean#

Returns whether any elements of an array match the given predicate. Returns true if one or more elements match the predicate; false if none of the elements matches (a special case is when the array is empty); NULL if the predicate function returns NULL for one or more elements and false for all other elements.

none_match(array(T), function(T, boolean)) → boolean#

Returns whether no elements of an array match the given predicate. Returns true if none of the elements matches the predicate (a special case is when the array is empty); false if one or more elements match; NULL if the predicate function returns NULL for one or more elements and false for all other elements.

netlify · 2023-03-17T12:35:05Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`650b047`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/641b32fcaef6740008fabc10

mbasmanova

@duanmeng Looks great % a couple of small comments.

velox/docs/functions/presto/array.rst

mbasmanova · 2023-03-21T07:08:25Z

velox/docs/functions/presto/array.rst

@@ -11,6 +11,15 @@ Array Functions
        Returns NULL if the predicate function returns NULL for one or more elements and true for all other elements.
        Throws an exception if the predicate fails for one or more elements and returns true or NULL for the rest.

+.. function:: any_match(array(T), function(T, boolean)) → boolean
+
+    Returns whether any elements of an array match the given predicate.


typo: "any elements" -> "at least one element"

mbasmanova · 2023-03-21T07:11:58Z

velox/functions/prestosql/ArrayMatch.cpp

+
+ protected:
+  static FOLLY_ALWAYS_INLINE bool hasError(
+      const ErrorVectorPtr& elementErrors,


nit: elementErrors -> errors

mbasmanova · 2023-03-21T07:36:46Z

velox/functions/prestosql/ArrayMatch.cpp

+      const vector_size_t* sizes,
+      const ErrorVectorPtr& elementErrors,
+      const exec::LocalDecodedVector& bitsDecoder) const override {
+    auto size = sizes[row];


A lot of this code repeats between the two functions. Also, virtual function call per row tends to be expensive. I wonder if we can reduce copy-paste by templating the implementation on a boolean. See https://github.com/facebookincubator/velox/compare/main...mbasmanova:velox-1:add_any_match?expand=1

mbasmanova · 2023-03-21T12:59:12Z

velox/docs/functions/presto/array.rst

@@ -11,6 +11,15 @@ Array Functions
        Returns NULL if the predicate function returns NULL for one or more elements and true for all other elements.
        Throws an exception if the predicate fails for one or more elements and returns true or NULL for the rest.

+.. function:: any_match(array(T), function(T, boolean)) → boolean
+
+    Returns whether at least one element of an array match the given predicate.


typo: match -> matches

mbasmanova

@duanmeng Looks good to me.

mbasmanova · 2023-03-21T12:59:41Z

velox/functions/prestosql/ArrayMatch.cpp

  void apply(
      const SelectivityVector& rows,
      std::vector<VectorPtr>& args,
-      const TypePtr& /*outputType*/,
+      const TypePtr& outputType,


If outputType is not used, it should be commented out. Otherwise, linter will complain.

mbasmanova · 2023-03-21T13:00:03Z

velox/functions/prestosql/ArrayMatch.cpp

-                .argumentType("array(T)")
-                .argumentType("function(T, boolean)")
-                .build()};
+ protected:


This can be private.

mbasmanova · 2023-03-21T13:02:04Z

velox/functions/prestosql/ArrayMatch.cpp

@@ -24,12 +24,13 @@
 namespace facebook::velox::functions {
 namespace {

-class AllMatchFunction : public exec::VectorFunction {
- public:
+template <bool match>


Let's add a comment to explain the 'match' template parameter. I wonder if there is a more descriptive name for this parameter.

There is another function: none_match. Can we reuse this class to implement it? What would need to change?

How about use enum class as the template parameter?

enum class MatchMethod { ALL = 0, ANY = 1, NONE = 2 }; template <MatchMethod mathMethod> class MatchFunction : public exec::VectorFunction { void applyInternal(...) const { bool match = true; if (mathMethod == MatchMethod::NONE) { // TODO: impl none_match } else { auto match = mathMethod == MatchMethod::ALL; // implementation of applyInternal in this pr } class AllMatchFunction : public MatchFunction<MatchMethod::ALL> {}; class AnyMatchFunction : public MatchFunction<MatchMethod::ANY> {}; class AllMatchFunction : public MatchFunction<MatchMethod::NONE> {}; // next pr

mbasmanova · 2023-03-21T13:02:40Z

velox/functions/prestosql/ArrayMatch.cpp

+
+    // Errors for individual array elements should be suppressed only if the
+    // outcome can be decided by some other array element, e.g. if there is
+    // another element that returns 'true' for the predicate.


This comment doesn't match the code.

mbasmanova · 2023-03-21T13:06:47Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+       {std::nullopt, std::nullopt}});
+  auto expectedResult =
+      makeNullableFlatVector<bool>({true, true, true, false, std::nullopt});
+  testExpr(expectedResult, "any_match(c0, x -> (x > 1))", input);


Can we shorten these tests by changing testExpr to take lambda expression and construct any_match(c0, x -> (<lambdaExpr>)):

testExpr(expectedResult, "x > 1", input);

Maybe shorten these tests further by passing std::vector<std::optional> as expected result:

expectedResult = {true, false, false, false, true}; testExpr(expectedResult, "x is null", input);

mbasmanova · 2023-03-21T13:09:17Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+      makeNullableFlatVector<bool>({true, false, true, std::nullopt});
+  testExpr(expectedResult, "any_match(c0, x -> x)", input);
+
+  auto emptyInput = makeArrayVector<int32_t>({{}});


It looks like this is already tested: L48

mbasmanova · 2023-03-21T13:10:09Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+      arrayOfArrays);
+}
+
+TEST_F(ArrayAnyMatchTest, bigints) {


Looks like this is already covered by 'basic' test.

facebook-github-bot · 2023-03-21T13:12:11Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova · 2023-03-21T18:06:46Z

velox/functions/prestosql/ArrayMatch.cpp

@@ -24,8 +24,11 @@
 namespace facebook::velox::functions {
 namespace {

-class AllMatchFunction : public exec::VectorFunction {
- public:
+enum class MatchMethod { ALL = 0, ANY = 1, NONE = 2 };


@duanmeng This is a reasonable idea, but how would we implement NONE case?

I'm seeing the the 3 cases need the following loops:

---- kAll ---- bool allMatch = true loop: if not match: allMatch = false; break; if (!allMatch) -> false else if hasError -> error else if hasNull -> null else -> true ---- kAny ---- bool anyMatch = false loop: if match: anyMatch = true; break; if (anyMatch) -> true else if hasError -> error else if hasNull -> null else -> false ---- kNone ---- bool noneMatch = true; loop: if match: noneMatch = false; break; if (!noneMatch) -> false else if hasError -> error else if hasNull -> null else -> true

These loops can be implemented as a single loop with 2 parameters: bool initialValue, bool earlyReturn.

See main...mbasmanova:velox-1:add_any_match

I suggest we include both any_match and none_match functions in this PR to make sure the code is generic enough to handle both.

@mbasmanova IMHO AllMatch and NoneMatch may have the same initialValue and earlyReturn and it is not descriptive to use them as template parameters. Should we use the enum and handle them in initial, flip, and result finalization?
All, none, and any match have different and similar logic intertwined in terms of the initial value, flip condition, and the result finalization:

Initial value:

All is true

Any is false

None is true

Flip logic:

All flips once encounter an unmatched element

Any flips once encounter a matched element

None flips once encounter a matched element

Result finalization:

All: ignore the error and null if one or more elements are unmatched and return false

Any: ignore the error and null if one or more elements matched and return true

None: ignore the error and null if one or more elements matched and return false

auto match = (matchMethod != MatchMethod::kAny); auto hasNull = false; std::exception_ptr errorPtr{nullptr}; for (auto i = 0; i < size; ++i) { auto idx = offset + i; if (hasError(elementErrors, idx)) { errorPtr = *std::static_pointer_cast<std::exception_ptr>( elementErrors->valueAt(idx)); continue; } if (bitsDecoder->isNullAt(idx)) { hasNull = true; } else if (matchMethod == MatchMethod::kAll) { if (!bitsDecoder->valueAt<bool>(idx)) { match = !match; break; } } else { if (bitsDecoder->valueAt<bool>(idx)) { match = !match; break; } } } if ((matchMethod == MatchMethod::kAny) == match) { flatResult->set(row, match); } else if (errorPtr) { context.setError(row, errorPtr); } else if (hasNull) { flatResult->setNull(row, true); } else { flatResult->set(row, match); }

@duanmeng Does the code I proposed not work? Would you add a test for none_match function?

@mbasmanova It works, I misunderstood the earlyReturn, which is the early return condition, not value. It's also more descriptive of those three match functions. I've changed it to your proposed codes. Thanks:)

mbasmanova · 2023-03-21T18:07:56Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+};
+
+TEST_F(ArrayAnyMatchTest, basic) {
+  auto input = makeNullableArrayVector<int64_t>(


Thank you for refactoring the tests. Much more readable. Should we go a bit further still?

TEST_F(ArrayAnyMatchTest, basic) { auto input = makeNullableArrayVector<int64_t>({ {std::nullopt, 2, 0}, {-1, 3}, {-2, -3}, {}, {0, std::nullopt}, }); testAnyMatch("x > 1", {true, true, false, false, std::nullopt}, input); testAnyMatch("x is null", {true, false, false, false, true}, input); input = makeNullableArrayVector<bool>( {{false, true}, {false, false}, {std::nullopt, true}, {std::nullopt, false}}); testAnyMatch("x", {true, false, true, std::nullopt}, input); }

mbasmanova · 2023-03-22T10:21:51Z

velox/docs/functions/presto/array.rst

@@ -11,6 +11,24 @@ Array Functions
        Returns NULL if the predicate function returns NULL for one or more elements and true for all other elements.
        Throws an exception if the predicate fails for one or more elements and returns true or NULL for the rest.

+.. function:: any_match(array(T), function(T, boolean)) → boolean
+
+    Returns whether at least one element of an array matchs the given predicate.


typo: matchs -> matches

mbasmanova · 2023-03-22T10:24:42Z

velox/functions/prestosql/ArrayMatch.cpp

+class AnyMatchFunction : public MatchFunction<MatchMethod::kAny> {};
+class NoneMatchFunction : public MatchFunction<MatchMethod::kNone> {};
+
+// TODO: add class NoneMatchFunction


Not needed.

I've added the NoneMatchFunction and its test.

mbasmanova · 2023-03-22T10:26:44Z

velox/functions/prestosql/ArrayMatch.cpp

@@ -24,8 +24,11 @@
 namespace facebook::velox::functions {
 namespace {

-class AllMatchFunction : public exec::VectorFunction {
- public:
+enum class MatchMethod { ALL = 0, ANY = 1, NONE = 2 };


@duanmeng Does the code I proposed not work? Would you add a test for none_match function?

mbasmanova

@duanmeng Looks great % a few small comments. Let's update PR title and description to include none_match function.

mbasmanova · 2023-03-22T12:44:08Z

velox/functions/prestosql/ArrayMatch.cpp

+  static FOLLY_ALWAYS_INLINE bool hasError(
+      const ErrorVectorPtr& errors,
+      int idx) {
+    return errors && idx < errors->size() && !errors->isNullAt(idx);
  }

 private:


This "private:" is not needed as there is already one above it.

mbasmanova · 2023-03-22T12:45:42Z

velox/functions/prestosql/ArrayMatch.cpp

+    }
+
+    if (result != initialValue) {
+      flatResult->set(row, !initialValue);


nit: perhaps, it would be more readable to do

flatResult->set(row, result);

mbasmanova · 2023-03-22T12:45:59Z

velox/functions/prestosql/ArrayMatch.cpp

+    } else if (hasNull) {
+      flatResult->setNull(row, true);
+    } else {
+      flatResult->set(row, initialValue);


and

flatResult->set(row, result);

mbasmanova · 2023-03-22T12:46:31Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+ * limitations under the License.
+ */
+
+#include <folly/Format.h>


Let's remove this and use fmt::format instead.

mbasmanova · 2023-03-22T12:47:16Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+class ArrayAnyMatchTest : public functions::test::FunctionBaseTest {
+ protected:
+  // Evaluate an expression.
+  void testExpr(


nit: testAnyMatch to distinguish from testExpr

mbasmanova · 2023-03-22T12:48:16Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+      const std::vector<std::optional<bool>>& expected,
+      const std::string& lambdaExpr,
+      const std::vector<std::vector<std::optional<T>>>& input) {
+    auto expression = folly::sformat("any_match(c0, x -> ({}))", lambdaExpr);


perhaps, testAnyMatch(expected, lambdaExpr, makeNullableArrayVector(input)) to reduce code duplication

mbasmanova · 2023-03-22T12:49:13Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+};
+
+TEST_F(ArrayAnyMatchTest, basic) {
+  std::vector<std::vector<std::optional<int32_t>>> ints{


Add a comma after last value in the vector for readability. This way each array value will appear on a separate line making it easier to understand.

Got it, thanks.

mbasmanova · 2023-03-22T12:50:14Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+  auto expression = "(10 / x) > 2";
+  std::vector<std::vector<std::optional<int8_t>>> input{
+      {0, 2, 0, 5, 0}, {2, 5, std::nullopt, 0}};
+  std::vector<std::optional<bool>> expectedResult = {true, true};


perhaps,

testAnyMatch({true, true}, "(10 / x) > 2", input);

mbasmanova · 2023-03-22T12:51:51Z

velox/functions/prestosql/tests/ArrayNoneMatchTest.cpp

+ * limitations under the License.
+ */
+
+#include <folly/Format.h>


same comments as for ArrayAnyMatchTest apply here

Also, let's refactor ArrayAllMatchTest in the same way to ensure all these are consistent.

mbasmanova · 2023-03-22T12:59:06Z

velox/functions/prestosql/ArrayMatch.cpp

+    auto size = sizes[row];
+    auto offset = offsets[row];
+
+    // All, none, and any match have different and similar logic intertwined in


I wonder if this comment is easy to understand.

Would it be helpful to show pseudo-code for different loops along with a generic loop?

all_match, none_match and any_match need to loop over predicate results for element arrays and check for results, nulls and errors. These loops can be generalized using two booleans. Here is what the individual loops look like. ---- kAll ---- bool allMatch = true loop: if not match: allMatch = false; break; if (!allMatch) -> false else if hasError -> error else if hasNull -> null else -> true ---- kAny ---- bool anyMatch = false loop: if match: anyMatch = true; break; if (anyMatch) -> true else if hasError -> error else if hasNull -> null else -> false ---- kNone ---- bool noneMatch = true; loop: if match: noneMatch = false; break; if (!noneMatch) -> false else if hasError -> error else if hasNull -> null else -> true To generalize these loops, we use initialValue and earlyReturn booleans like so: --- generic loop --- bool result = initialValue loop: if match == earlyReturn: result = false; break; if (result != initialValue) -> result else if hasError -> error else if hasNull -> null else -> result

duanmeng · 2023-03-22T16:41:05Z

@mbasmanova I've resolved all the comments and refactored AllMatchFunctionTest, could you help to take a look?

mbasmanova

@duanmeng Looks great. Let's update PR title and description to add none_match function.

mbasmanova · 2023-03-22T16:49:49Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

-class ArrayAllMatchTest : public functions::test::FunctionBaseTest {};
+class ArrayAllMatchTest : public functions::test::FunctionBaseTest {
+ protected:
+  // Evaluate an expression.


nit: This comment is redundant. Let's remove.

mbasmanova · 2023-03-22T16:50:38Z

velox/functions/prestosql/tests/ArrayAnyMatchTest.cpp

+
+class ArrayAnyMatchTest : public functions::test::FunctionBaseTest {
+ protected:
+  // Evaluate an expression.


mbasmanova · 2023-03-22T16:50:57Z

velox/functions/prestosql/tests/ArrayNoneMatchTest.cpp

+
+class ArrayNoneMatchTest : public functions::test::FunctionBaseTest {
+ protected:
+  // Evaluate an expression.


facebook-github-bot · 2023-03-22T16:51:55Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-22T16:57:20Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-22T22:49:45Z

@mbasmanova merged this pull request in e7d6a1b.

Summary: any_match(array(T), function(T, boolean)) → boolean[#](https://prestodb.io/docs/current/functions/array.html#any_match) >Returns whether any elements of an array match the given predicate. Returns true if one or more elements match the predicate; false if none of the elements matches (a special case is when the array is empty); NULL if the predicate function returns NULL for one or more elements and false for all other elements. none_match(array(T), function(T, boolean)) → boolean[#](https://prestodb.io/docs/current/functions/array.html#none_match) >Returns whether no elements of an array match the given predicate. Returns true if none of the elements matches the predicate (a special case is when the array is empty); false if one or more elements match; NULL if the predicate function returns NULL for one or more elements and false for all other elements. Pull Request resolved: facebookincubator#4327 Reviewed By: Yuhta Differential Revision: D44255945 Pulled By: mbasmanova fbshipit-source-id: f3797a83723badc33a644de7b76baae3e54df3cc

duanmeng requested a review from mbasmanova March 17, 2023 12:35

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 17, 2023

duanmeng marked this pull request as draft March 17, 2023 12:35

duanmeng force-pushed the add_any_match branch from 3597731 to 22b49d4 Compare March 17, 2023 12:40

duanmeng removed the request for review from mbasmanova March 17, 2023 12:42

duanmeng self-assigned this Mar 17, 2023

duanmeng mentioned this pull request Mar 18, 2023

Add Presto functions #2262

Open

duanmeng force-pushed the add_any_match branch from 22b49d4 to c451835 Compare March 21, 2023 03:51

duanmeng marked this pull request as ready for review March 21, 2023 03:52

duanmeng requested a review from mbasmanova March 21, 2023 03:52

add any_match function

0134a1e

duanmeng force-pushed the add_any_match branch from c451835 to 0134a1e Compare March 21, 2023 05:43

mbasmanova reviewed Mar 21, 2023

View reviewed changes

resolve comments

83dcfab

mbasmanova reviewed Mar 21, 2023

View reviewed changes

mbasmanova approved these changes Mar 21, 2023

View reviewed changes

duanmeng force-pushed the add_any_match branch from 1245c3a to e781eb9 Compare March 21, 2023 16:13

resolve comments

4bb611d

duanmeng force-pushed the add_any_match branch from e781eb9 to 4bb611d Compare March 21, 2023 16:24

mbasmanova reviewed Mar 21, 2023

View reviewed changes

duanmeng force-pushed the add_any_match branch 3 times, most recently from 864642b to 6a3eac1 Compare March 22, 2023 08:25

mbasmanova reviewed Mar 22, 2023

View reviewed changes

duanmeng force-pushed the add_any_match branch from 6a3eac1 to c589d04 Compare March 22, 2023 12:13

resolve comments

1941a79

duanmeng force-pushed the add_any_match branch from c589d04 to 1941a79 Compare March 22, 2023 12:17

use bool flags

13d267e

mbasmanova reviewed Mar 22, 2023

View reviewed changes

mbasmanova approved these changes Mar 22, 2023

View reviewed changes

duanmeng changed the title ~~Add any_match Presto function~~ Add any_match, none_match Presto function Mar 22, 2023

duanmeng changed the title ~~Add any_match, none_match Presto function~~ Add any_match, none_match Presto functions Mar 22, 2023

mbasmanova changed the title ~~Add any_match, none_match Presto functions~~ Add any_match and none_match Presto functions Mar 22, 2023

resolve comments

650b047

duanmeng force-pushed the add_any_match branch from 57f2f6a to 650b047 Compare March 22, 2023 16:55

facebook-github-bot closed this in e7d6a1b Mar 22, 2023

facebook-github-bot added the Merged label Mar 22, 2023

Add any_match and none_match Presto functions #4327

Add any_match and none_match Presto functions #4327

Conversation

duanmeng commented Mar 17, 2023 • edited Loading

netlify bot commented Mar 17, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 21, 2023

Choose a reason for hiding this comment

duanmeng Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duanmeng commented Mar 22, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

facebook-github-bot commented Mar 22, 2023

duanmeng commented Mar 17, 2023 •

edited

Loading

netlify bot commented Mar 17, 2023 •

edited

Loading

duanmeng Mar 22, 2023 •

edited

Loading