Add all_match Presto function #3356

duanmeng · 2022-11-24T11:19:33Z

all_match(array(T), function(T, boolean)) → boolean

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

netlify · 2022-11-24T11:19:41Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`16a4a1c`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6414854e6f18f800089f697d

isadikov · 2023-01-27T05:44:17Z

Hi @duanmeng. Are you still working on this function?

duanmeng · 2023-01-27T05:59:36Z

Hi @duanmeng. Are you still working on this function?

@isadikov Yes, sorry for the delay. I recovered from the covid-19, and am on the lunar new year holidays this month. I will resolve the conflicts and comments if someone helps to review:), in 3/4 days.

isadikov

Thank you for opening the PR and working on the function! I left a few comments, I would appreciate it if you could take a look.

velox/functions/prestosql/ArrayAllMatch.cpp

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

velox/functions/prestosql/ArrayAllMatch.cpp

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

duanmeng · 2023-02-07T02:03:32Z

@isadikov I've resolved all the comments, could you help to have a look？

isadikov

Looks good, thanks for working on this. I left a couple of nit comments.

velox/functions/prestosql/ArrayAllMatch.cpp

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

duanmeng · 2023-02-13T01:57:32Z

@isadikov I've resolved the rest of nit comments, could you help to have a look？

isadikov · 2023-02-13T05:57:32Z

Looks good, thanks for working on this!

@mbasmanova Could you take a look and review this PR that adds all_match function? Thank you!

duanmeng · 2023-02-21T05:56:17Z

@mbasmanova Could you please help to take a look, I've resolved all the comments, thanks.

mbasmanova

@duanmeng Looks good overall. Some comments below.

velox/docs/functions/presto/array.rst

velox/functions/prestosql/ArrayAllMatch.cpp

duanmeng · 2023-03-06T16:40:17Z

@mbasmanova I've resolved all the comments and added the error handling logic and related UT, could you help to take a look?

velox/docs/functions/presto/array.rst

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

velox/functions/prestosql/ArrayAllMatch.cpp

mbasmanova

@duanmeng Thank you for iterating on this PR. Looks good to me % a couple comments.

mbasmanova · 2023-03-08T07:50:18Z

velox/docs/functions/presto/array.rst

+        Returns true if all the elements match the predicate (a special case is when the array is empty);
+        Returns false if one or more elements don’t match;
+        Returns NULL if the predicate function returns NULL for one or more elements and true for all other elements.
+        Throw exceptions if the predicate function has errors on some elements and returns true or NULL on others.


typos: Throws an exception if the predicate fails for one or more elements and returns true or NULL for the rest.

mbasmanova · 2023-03-08T07:52:40Z

velox/functions/prestosql/ArrayAllMatch.cpp

+        auto allMatch = true;
+        auto hasNull = false;
+        auto hasError =
+            errors && row < errors->size() && !errors->isNullAt(row);


perhaps, add a helper method to EvalCtx for readability:

auto hasError = context.hasError(row);

Also, this variable is not used until late. Let's move it next to first use.

mbasmanova · 2023-03-08T07:57:56Z

velox/functions/prestosql/ArrayAllMatch.cpp

+          if (hasError) {
+            auto err = std::static_pointer_cast<std::exception_ptr>(
+                context.errors()->valueAt(row));
+            std::rethrow_exception(*err);


We need to make sure we throw VeloxException. See throwError in EvalCtx.cpp.

auto throwError(const std::exception_ptr& exceptionPtr) { std::rethrow_exception(toVeloxException(exceptionPtr)); }

Consider moving this logic into a helper method in EvalCtx to avoid copy-paste.

context.throwIfHasError(row);

mbasmanova · 2023-03-08T07:58:28Z

velox/functions/prestosql/ArrayAllMatch.cpp

+    exec::LocalDecodedVector bitsDecoder(context);
+    auto it = args[1]->asUnchecked<FunctionVector>()->iterator(&rows);
+
+    // Turns off the ThrowOnError flag and temporarily resets errors in the


Use active verbs: Turn off ... and reset

mbasmanova · 2023-03-08T07:59:50Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+  auto result = evaluate<SimpleVector<bool>>(
+      "all_match(c0, x -> ((10 / x) > 2))", makeRowVector({input}));
+
+  auto expectedResult = makeNullableFlatVector<bool>({false, false});


makeNullableFlatVector -> makeFlatVector

mbasmanova · 2023-03-08T08:00:07Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+}
+
+TEST_F(ArrayAllMatchTest, errorReThrow) {
+  static constexpr std::string_view errorMessage{"division by zero"};


naming: kErrorMessage and kErrorCode

mbasmanova · 2023-03-08T08:00:17Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+  static constexpr std::string_view errorMessage{"division by zero"};
+  static constexpr std::string_view errorCode{"ARITHMETIC_ERROR"};
+
+  try {


use VELOX_ASSERT_THROW

Let's also verify that these expressions work properly with TRY.

TRY(all_match(c0, x -> ))

all_match(c0, x -> TRY()))

mbasmanova · 2023-03-16T03:01:52Z

velox/expression/LambdaExpr.cpp

+      const BufferPtr& wrapCapture,
+      EvalCtx* context,
+      const std::vector<VectorPtr>& args,
+      VectorPtr& errors,


VectorPtr -> ErrorVectorPtr (sorry, I got it wrong in my earlier suggestion)

mbasmanova

@duanmeng Looks great % minor comments.

mbasmanova · 2023-03-16T19:13:59Z

velox/expression/EvalCtx.h

+  }
+
+  /// Extract the actual exception of the row
+  std::exception_ptr getError(vector_size_t row) {


These new methods seem unused. Let's remove.

mbasmanova · 2023-03-16T19:14:10Z

velox/expression/LambdaExpr.cpp

-        rows.end(),
-        std::move(allVectors));
-    EvalCtx lambdaCtx(context->execCtx(), context->exprSet(), row.get());
+    auto row = createRowVector(context, wrapCapture, args, rows.end());


Nice refactor.

mbasmanova · 2023-03-16T19:14:37Z

velox/expression/LambdaExpr.cpp

+    transformErrorVector(lambdaCtx, context, rows, elementToTopLevelRows);
+  }
+
+  void applyNoThrowError(


applyNoThrowError -> applyNoThrow for consistency (see EvalCtx::applyToSelectedNoThrow)

mbasmanova · 2023-03-16T19:15:48Z

velox/functions/prestosql/ArrayAllMatch.cpp

+        auto allMatch = true;
+        auto hasNull = false;
+        std::exception_ptr errorPtr = nullptr;
+        auto hasError = false;


Maybe we don't need hasError variable. We can just check if errorPtr is null or not.

mbasmanova · 2023-03-16T19:16:13Z

velox/functions/prestosql/ArrayAllMatch.cpp

+        auto hasError = false;
+        for (auto i = 0; i < size; ++i) {
+          auto idx = offset + i;
+          if (elementErrors && idx < elementErrors->size() &&


nit: consider extracting a helper function for readability

if (hasError(elementErrors, idx))

mbasmanova · 2023-03-16T19:20:47Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+  assertEqualVectors(expectedResult, result);
+}
+
+TEST_F(ArrayAllMatchTest, errorSuppress) {


errorSuppress -> errors

mbasmanova · 2023-03-16T19:21:07Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+  assertEqualVectors(expectedResult, result);
+}
+
+TEST_F(ArrayAllMatchTest, errorReThrow) {


maybe combine this test case with the previous one

mbasmanova · 2023-03-16T19:21:12Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+      kErrorMessage);
+}
+
+TEST_F(ArrayAllMatchTest, withTrys) {


velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

mbasmanova · 2023-03-16T19:22:26Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+      makeRowVector({makeArrayVector<int8_t>({{1, 0}, {1}, {6}})}));
+  expectedResult = makeNullableFlatVector<bool>({std::nullopt, true, false});
+  assertEqualVectors(expectedResult, result);
+}


Let's add a test case with multiple lambdas. See TransformTest.cpp:

// Test different lambdas applied to different rows TEST_F(TransformTest, conditional) {

duanmeng · 2023-03-17T12:50:21Z

@mbasmanova I've resolved all comments, combined the error test cases, and added conditional multiple lambdas test cases, could you please help to have a look?

mbasmanova

@duanmeng Looks great. Thank you for your patience.

velox/vector/FunctionVector.h

mbasmanova · 2023-03-17T13:16:42Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+      kErrorMessage);
+  VELOX_ASSERT_THROW(
+      evaluate<SimpleVector<bool>>(
+          "all_match(c0, x -> ((10 / x) > 2))", makeRowVector({errorInput})),


nit: consider changing errorInput to include makeRowVector, so it can be used directly and drop template parameter for readability:

VELOX_ASSERT_THROW( evaluate("all_match(c0, x -> ((10 / x) > 2))", errorInput),

facebook-github-bot · 2023-03-17T13:17:52Z

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

mbasmanova · 2023-03-17T14:18:12Z

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp

+  auto input = makeRowVector({makeNullableArrayVector<int8_t>(
+      {{0, 2, 0, 5, 0}, {5, std::nullopt, 0}})});
+  auto result =
+      evaluate<SimpleVector<bool>>("all_match(c0, x -> ((10 / x) > 2))", input);


nit: can drop template parameters here and in other similar places as well

facebook-github-bot · 2023-03-17T14:19:02Z

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

facebook-github-bot · 2023-03-17T14:47:56Z

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

mbasmanova · 2023-03-17T14:51:42Z

velox/vector/FunctionVector.h

@@ -15,14 +15,11 @@
 */
 #pragma once

+#include "velox/expression/EvalCtx.h"


Looks like this introduces circular dependency: vectors depend expressions and expressions depend on vectors. We need to figure out how to break that.

Maybe move ErrorVectorPtr to FlatVector.h.

using ErrorVector = FlatVector<std::shared_ptr<void>>; using ErrorVectorPtr = std::shared_ptr<ErrorVector>;

Sure, done. ErrorVectorPtr should be declared in the vectors scope.

facebook-github-bot · 2023-03-17T15:21: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-21T02:00:23Z

@mbasmanova merged this pull request in d8c747f.

mbasmanova · 2023-03-21T07:06:38Z

@duanmeng Now that the all_match Presto function is merged into Velox, it would be nice to follow up with an e2e test case in Prestissimo to ensure this function works correctly end-to-end: presto-native-execution/src/test/java/com/facebook/presto/nativeworker/TestHiveArrayFunctionQueries.java

Summary: all_match(array(T), function(T, boolean)) → boolean >Returns whether all elements of an array match the given predicate. Returns true if all the elements match the predicate (a special case is when the array is empty); false if one or more elements don’t match; NULL if the predicate function returns NULL for one or more elements and true for all other elements. Pull Request resolved: facebookincubator#3356 Reviewed By: Yuhta Differential Revision: D44165931 Pulled By: mbasmanova fbshipit-source-id: a23bc2619c3d2befcaf2aab9a373415ecc8f378a

…d of all partitions (facebookincubator#3356)

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 Nov 24, 2022

duanmeng force-pushed the add_all_match branch from ebfca34 to 24d346c Compare November 24, 2022 11:45

darrenfu mentioned this pull request Jan 20, 2023

Add Presto functions #2262

Open

isadikov reviewed Jan 29, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch 3 times, most recently from 336e301 to 587f8a1 Compare February 6, 2023 13:02

isadikov approved these changes Feb 7, 2023

View reviewed changes

velox/functions/prestosql/ArrayAllMatch.cpp Show resolved Hide resolved

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp Show resolved Hide resolved

mbasmanova changed the title ~~Add all_match Presto Functions~~ Add all_match Presto function Feb 28, 2023

mbasmanova reviewed Feb 28, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch 5 times, most recently from edae9dd to 731f937 Compare March 4, 2023 17:11

mbasmanova reviewed Mar 7, 2023

View reviewed changes

velox/docs/functions/presto/array.rst Show resolved Hide resolved

mbasmanova reviewed Mar 7, 2023

View reviewed changes

velox/functions/prestosql/tests/ArrayAllMatchTest.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Mar 7, 2023

View reviewed changes

velox/functions/prestosql/ArrayAllMatch.cpp Outdated Show resolved Hide resolved

duanmeng force-pushed the add_all_match branch 2 times, most recently from 212f8fb to c394eb4 Compare March 8, 2023 04:53

mbasmanova reviewed Mar 8, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch from e8a3e16 to 69d0ffb Compare March 8, 2023 13:24

resolve comments

ea39917

mbasmanova reviewed Mar 16, 2023

View reviewed changes

use elements error ptr

ac445a9

mbasmanova reviewed Mar 16, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch from 2e92cb9 to 07439fe Compare March 17, 2023 11:54

resolve comments

118cc5c

duanmeng force-pushed the add_all_match branch from 07439fe to 118cc5c Compare March 17, 2023 12:13

duanmeng self-assigned this Mar 17, 2023

mbasmanova approved these changes Mar 17, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch from 6e13f84 to bbfddfc Compare March 17, 2023 14:08

mbasmanova reviewed Mar 17, 2023

View reviewed changes

resolve comments

9e573ab

duanmeng force-pushed the add_all_match branch from bbfddfc to 9e573ab Compare March 17, 2023 14:32

mbasmanova reviewed Mar 17, 2023

View reviewed changes

duanmeng force-pushed the add_all_match branch from edc6ca2 to 6321e74 Compare March 17, 2023 15:19

adjust ErrorVectorPtr

16a4a1c

duanmeng force-pushed the add_all_match branch from 6321e74 to 16a4a1c Compare March 17, 2023 15:20

facebook-github-bot closed this in d8c747f Mar 21, 2023

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

duanmeng mentioned this pull request Apr 6, 2023

[native] Add e2e test for all/any/none_match functions. prestodb/presto#19348

Merged

marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023

[VL] Refine velox shuffle writer iteration using partitionUsed instea…

b05f5d8

…d of all partitions (facebookincubator#3356)

Add all_match Presto function #3356

Add all_match Presto function #3356

Conversation

duanmeng commented Nov 24, 2022 • edited by mbasmanova Loading

netlify bot commented Nov 24, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

isadikov commented Jan 27, 2023

duanmeng commented Jan 27, 2023 • edited Loading

isadikov left a comment

Choose a reason for hiding this comment

duanmeng commented Feb 7, 2023 • edited Loading

isadikov left a comment

Choose a reason for hiding this comment

duanmeng commented Feb 13, 2023

isadikov commented Feb 13, 2023

duanmeng commented Feb 21, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

duanmeng commented Mar 6, 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

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 Mar 8, 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

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

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 17, 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

facebook-github-bot commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 17, 2023

facebook-github-bot commented Mar 17, 2023

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 17, 2023

facebook-github-bot commented Mar 21, 2023

mbasmanova commented Mar 21, 2023

duanmeng commented Nov 24, 2022 •

edited by mbasmanova

Loading

netlify bot commented Nov 24, 2022 •

edited

Loading

duanmeng commented Jan 27, 2023 •

edited

Loading

duanmeng commented Feb 7, 2023 •

edited

Loading

mbasmanova Mar 8, 2023 •

edited

Loading