Add array_average Presto function #2434

jwyles-ahana · 2022-08-31T20:10:20Z

Implementing array_average as a simple function.

netlify · 2022-08-31T20:10:26Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`9dcb718`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/634db7b39b586900080937f6

mbasmanova

@jwyles-ahana Thank you for adding one more Presto function. Please, add documentation and look into CI failures.

kevinwilfong

Thanks for contributing this!

kevinwilfong · 2022-08-31T20:18:53Z

velox/functions/prestosql/ArrayFunctions.h

+  }
+
+  template <typename TOutput, typename TInput>
+  FOLLY_ALWAYS_INLINE void callNullFree(TOutput& out, const TInput& array) {


To be consistent with the implementation of "call" and the Presto documentation this should return null if the input array is empty, so the return type should be bool.

Change made.

velox/functions/prestosql/tests/ArrayAverageTest.cpp

aditi-pandit

Thanks for adding this function. Please add documentation in array.rst.

aditi-pandit · 2022-09-02T00:07:15Z

velox/functions/prestosql/registration/ArrayFunctionsRegistration.cpp

@@ -30,6 +30,11 @@ inline void registerArraySumFunction() {
  registerFunction<ArraySumFunction, OT, IT>({"array_sum_alt"});
 }

+template <typename OT, typename IT>


Nit: Since we don't bind this function for multiple types, the usage of template variables doesn't seem worthwhile to me.

I have removed the registerArrayAverageFunction() method.

velox/functions/prestosql/tests/ArrayAverageTest.cpp

laithsakka · 2022-09-06T16:07:42Z

velox/functions/prestosql/ArrayFunctions.h

+  template <typename TOutput, typename TInput>
+  FOLLY_ALWAYS_INLINE bool call(TOutput& out, const TInput& array) {
+    TOutput sum = 0;
+    int count = 0;


nit: int64_t or size_t?

Changed to size_t

laithsakka · 2022-09-06T16:08:35Z

velox/functions/prestosql/ArrayFunctions.h

+template <typename T>
+struct ArrayAverageFunction {
+  VELOX_DEFINE_FUNCTION_TYPES(T)
+  template <typename TOutput, typename TInput>


TOutput is always double.

Rewrote call and callNullFree methods as non-templated.

jwyles-ahana · 2022-09-20T22:44:00Z

@mbasmanova @laithsakka What else needs to be done for this PR?

laithsakka · 2022-10-01T04:35:22Z

velox/functions/prestosql/ArrayFunctions.h

+      double& out,
+      const arg_type<Array<double>>& array) {
+    // If the array is empty then set result to null
+    if (array.size() == 0)


laithsakka

looks good just one minor missing braces before i import it.

facebook-github-bot · 2022-10-01T04:45:59Z

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

facebook-github-bot · 2022-10-03T19:58:54Z

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

mbasmanova · 2022-10-04T15:10:30Z

velox/functions/prestosql/tests/ArrayAverageTest.cpp

+  using limits = std::numeric_limits<double>;
+  auto input = makeNullableArrayVector<double>(
+      {{limits::infinity(), 1.0, 2.0},
+       {limits::min(), 1.0, 2.0},


@jwyles-ahana There is a linter warning. Would you double check?

Use of std::numeric_limits<T>::min() on floating-point type T. Use std::numeric_limits<T>::lowest() instead ofstd::numeric_limits<T>::min() where T is a floating-point type. std::numeric_limits<T>min() behaves counter-intuitively when T isa floating-point type (e.g. float, double, long double, etc.). Thismost likely does not do what the author intends. Lint code: CLANGTIDY Lint name: facebook-hte-FloatingPointMin - {limits::min(), 1.0, 2.0}, + {limits::lowest(), 1.0, 2.0},

mbasmanova · 2022-10-17T11:02:53Z

@jwyles-ahana This PR is almost complete, but there is a linter warning. Any chance you could fix it so we could merge this PR?

jwyles-ahana · 2022-10-17T22:13:42Z

@mbasmanova @laithsakka I have made the requested changes and now all checks are passing except the conbench one. I am not sure how anything in this commit could affect any of the existing benchmarks... Any ideas on what to look for to fix this?

mbasmanova · 2022-10-17T22:37:39Z

@jwyles-ahana Thanks. conbench is known to be flaky. It is fine to ignore the failures. CC: @pedroerp @raulcd @kgpai

@laithsakka Laith, would you re-import and land this PR?

mbasmanova · 2022-11-02T00:35:42Z

@jwyles-ahana @laithsakka Folks, anything blocking this PR?

kgpai · 2023-01-03T18:35:38Z

@jwyles-ahana Can you update with latest ? Should be good to merge/land after that .

isadikov · 2023-01-18T18:44:40Z

@aditi-pandit I can take over the function. Maybe we can close this PR and I will open a new one with the same changes (I cannot push directly to this fork).

aditi-pandit · 2023-01-18T18:52:54Z

@aditi-pandit I can take over the function. Maybe we can close this PR and I will open a new one with the same changes (I cannot push directly to this fork).

@isadikov : Yes, that is fine. We had contacted James, but didn't hear back. Please refernce this original PR in your new one.

Summary: Based on #2434. Pull Request resolved: #3741 Reviewed By: pedroerp Differential Revision: D42705940 Pulled By: mbasmanova fbshipit-source-id: 22105ec895fb93a30dc4c0b3d54b0afeff418240

stale · 2023-04-18T20:02:59Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

isadikov · 2023-04-18T20:16:28Z

I am still working on this.

stale · 2023-07-17T22:04:07Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

…sues (facebookincubator#2434)

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 Aug 31, 2022

mbasmanova requested a review from laithsakka August 31, 2022 20:16

mbasmanova reviewed Aug 31, 2022

View reviewed changes

jwyles-ahana force-pushed the array_average branch from d47cdcb to fa75cd1 Compare August 31, 2022 20:21

kevinwilfong reviewed Aug 31, 2022

View reviewed changes

jwyles-ahana force-pushed the array_average branch from fa75cd1 to ba8519d Compare September 1, 2022 22:09

aditi-pandit reviewed Sep 2, 2022

View reviewed changes

laithsakka reviewed Sep 6, 2022

View reviewed changes

jwyles-ahana force-pushed the array_average branch 5 times, most recently from 2a604d5 to 0e029ef Compare September 12, 2022 21:56

laithsakka reviewed Oct 1, 2022

View reviewed changes

laithsakka approved these changes Oct 1, 2022

View reviewed changes

mbasmanova reviewed Oct 4, 2022

View reviewed changes

mbasmanova mentioned this pull request Oct 17, 2022

Add Presto functions #2262

Open

jwyles-ahana force-pushed the array_average branch 3 times, most recently from 40b74b6 to 1cb0e54 Compare October 17, 2022 20:12

Add array_average Presto function

9dcb718

jwyles-ahana force-pushed the array_average branch from 1cb0e54 to 9dcb718 Compare October 17, 2022 20:14

isadikov mentioned this pull request Jan 18, 2023

Add array_average Presto function #3741

Closed

stale bot added the stale label Apr 18, 2023

stale bot removed the stale label Apr 18, 2023

stale bot added the stale label Jul 17, 2023

stale bot closed this Jul 31, 2023

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

[GLUTEN-2219] Apply an auto-generated patch for fixing code format is…

3c694c7

…sues (facebookincubator#2434)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add array_average Presto function #2434

Add array_average Presto function #2434

jwyles-ahana commented Aug 31, 2022

netlify bot commented Aug 31, 2022 •

edited

Loading

mbasmanova left a comment

kevinwilfong left a comment

kevinwilfong Aug 31, 2022

jwyles-ahana Sep 6, 2022

aditi-pandit left a comment

aditi-pandit Sep 2, 2022

mbasmanova Sep 6, 2022

jwyles-ahana Sep 6, 2022

laithsakka Sep 6, 2022

jwyles-ahana Sep 6, 2022

laithsakka Sep 6, 2022

jwyles-ahana Sep 6, 2022

jwyles-ahana commented Sep 20, 2022

laithsakka Oct 1, 2022

laithsakka left a comment

facebook-github-bot commented Oct 1, 2022

facebook-github-bot commented Oct 3, 2022

mbasmanova Oct 4, 2022

mbasmanova commented Oct 17, 2022

jwyles-ahana commented Oct 17, 2022

mbasmanova commented Oct 17, 2022

mbasmanova commented Nov 2, 2022

kgpai commented Jan 3, 2023

isadikov commented Jan 18, 2023

aditi-pandit commented Jan 18, 2023

stale bot commented Apr 18, 2023

isadikov commented Apr 18, 2023

stale bot commented Jul 17, 2023

Add array_average Presto function #2434

Add array_average Presto function #2434

Conversation

jwyles-ahana commented Aug 31, 2022

netlify bot commented Aug 31, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova left a comment

Choose a reason for hiding this comment

kevinwilfong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit 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

jwyles-ahana commented Sep 20, 2022

Choose a reason for hiding this comment

laithsakka left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 1, 2022

facebook-github-bot commented Oct 3, 2022

Choose a reason for hiding this comment

mbasmanova commented Oct 17, 2022

jwyles-ahana commented Oct 17, 2022

mbasmanova commented Oct 17, 2022

mbasmanova commented Nov 2, 2022

kgpai commented Jan 3, 2023

isadikov commented Jan 18, 2023

aditi-pandit commented Jan 18, 2023

stale bot commented Apr 18, 2023

isadikov commented Apr 18, 2023

stale bot commented Jul 17, 2023

netlify bot commented Aug 31, 2022 •

edited

Loading