feat: Add companionFunction to function metadata #9250

pramodsatya · 2024-03-26T05:17:51Z

Adds a boolean field, isCompanionFunction, to VectorFunctionMetadata,
AggregateFunctionMetadata, and WindowFunction::Metadata, to indicate
whether the respective scalar, aggregate, and window functions are companion
functions in Velox.
This field would be used to check for and exclude companion functions from
the function metadata returned by the v1/functions endpoint in the Presto
C++ sidecar. Currently, this is being done by searching for specific suffixes in
the registered companion functions' names.
Related discussion: #11011 .

netlify · 2024-03-26T05:18:11Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`01de22d`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/679e57fa477ce10008b5d5f7

velox/functions/CoverageUtil.h

velox/functions/FunctionRegistry.h

velox/functions/FunctionRegistry.cpp

velox/functions/CoverageUtil.h

velox/functions/FunctionRegistry.cpp

aditi-pandit · 2024-03-26T23:44:09Z

@pramodsatya : How do we take care that we get only Prestissimo functions from the registry ? If there is a conflict between a Prestissimo and Spark function with the same name, then how do we disambiguate ?

pramodsatya · 2024-03-27T19:31:21Z

Thanks for the feedback @aditi-pandit, @czentgr. We are not checking that only Prestissimo functions are retrieved from the registry. Instead we rely on the Prestissimo worker to have registered only the presto functions, such as in this function, so there are no Spark functions in the registry. Please let me know if this is fine or whether we should have an additional way to distinguish between Presto and Spark functions in the registry.

pramodsatya · 2024-06-07T16:11:57Z

Hi @aditi-pandit, @czentgr, could you please take another look at this PR?

velox/functions/CMakeLists.txt

velox/functions/FunctionRegistry.cpp

pramodsatya

Thanks for the feedback @aditi-pandit, addressed the comments. Could you please take another look?

velox/functions/CMakeLists.txt

velox/functions/FunctionRegistry.cpp

aditi-pandit

Thanks @pramodsatya

velox/functions/FunctionRegistry.cpp

velox/functions/FunctionRegistry.h

velox/functions/tests/FunctionRegistryTest.cpp

czentgr

Thanks, looks good now.

czentgr · 2024-08-29T22:17:45Z

velox/exec/AggregateCompanionAdapter.cpp

@@ -413,10 +423,11 @@ bool CompanionFunctionsRegistrar::registerMergeExtractFunction(

  auto mergeExtractFunctionName =
      CompanionSignatures::mergeExtractFunctionName(name);
-  return registerAggregateFunction(
+  return registerMergeExtractFunctionImpl(


Maybe rename to registerMergeExtractFunctionInternal.

aditi-pandit · 2024-08-30T07:16:47Z

@pramodsatya. @czentgr : This is a reasonable solution as well, if we are okay exposing companion function concept to the services using Velox.

With the other API to return all functions with a boolean parameter to say 'skipInternalFunctions' a single function could be used to skip all companion or any other internal functions added by the Velox framework.

@mbasmanova : wdyt ?

mbasmanova

@pramodsatya Pramod, would you update PR description to explain this change?

pramodsatya · 2024-10-25T18:33:22Z

@pramodsatya Pramod, would you update PR description to explain this change?

Apologies @mbasmanova, since this PR went through many iterations I had left it empty until the related discussion was concluded. I have updated the description now, could you please take another look?

mbasmanova · 2024-11-22T12:13:28Z

@pramodsatya

whether the respective scalar, aggregate, and window functions are companion functions

Can a window function be a companion function? I assume not. If so, let's remove isCompanion function from window function metadata.

mbasmanova · 2024-10-24T19:45:36Z

velox/expression/FunctionMetadata.h

@@ -59,6 +62,11 @@ class VectorFunctionMetadataBuilder {
    return *this;
  }

+  VectorFunctionMetadataBuilder& isCompanionFunction(bool isCompanionFunction) {


naming: drop 'is';

builder.companionFunction(true)

naming: drop 'is';

mbasmanova · 2024-11-22T12:15:32Z

velox/exec/Aggregate.cpp

+  if (auto func = getAggregateFunctionEntry(sanitizedName)) {
+    return func->metadata;
+  } else {
+    VELOX_USER_FAIL("Metadata not found for aggregate function: {}", name);


The issue here is that aggregate function doesn't exist, not that it is missing metadata. Let's clarify.

VELOX_USER_FAIL("Aggregate function not found: {}", name);

mbasmanova · 2024-11-22T12:15:39Z

velox/exec/Aggregate.cpp

+  const auto sanitizedName = sanitizeName(name);
+  if (auto func = getAggregateFunctionEntry(sanitizedName)) {
+    return func->metadata;
+  } else {


nit: drop else after return

mbasmanova · 2024-11-22T12:18:17Z

velox/exec/WindowFunction.h

@@ -43,14 +43,15 @@ class WindowFunction {
    kRows,
  };

-  /// Indicates whether this is an aggregate window function and its process
-  /// unit.
+  /// Indicates whether this is an aggregate window function, whether it is a


nit:

...whether the corresponding aggregate function is a companion function....

mbasmanova · 2024-11-22T12:18:35Z

velox/exec/WindowFunction.h

  struct Metadata {
    ProcessMode processMode;
    bool isAggregate;
+    bool isCompanionFunction;


Perhaps, clarify that this can be true iff isAggregate is true.

mbasmanova · 2024-11-22T12:20:16Z

velox/expression/FunctionMetadata.h

@@ -40,6 +40,9 @@ struct VectorFunctionMetadata {
  /// In this case, 'rows' in VectorFunction::apply will point only to positions
  /// for which all arguments are not null.
  bool defaultNullBehavior{true};
+
+  /// Indicates if this is a companion function.
+  bool isCompanionFunction{false};


consistency: drop 'is' to math other booleans here

mbasmanova · 2024-11-22T12:20:31Z

velox/exec/Aggregate.h

@@ -476,6 +476,9 @@ struct AggregateFunctionMetadata {
  /// True if results of the aggregation depend on the order of inputs. For
  /// example, array_agg is order sensitive while count is not.
  bool orderSensitive{true};
+
+  /// Indicates if this is a companion function.
+  bool isCompanionFunction{false};


consistency: drop 'is' to math the other boolean

mbasmanova · 2024-11-22T12:22:09Z

velox/functions/lib/window/RowNumber.cpp

@@ -75,7 +75,7 @@ void registerRowNumber(const std::string& name, TypeKind resultTypeKind) {
  exec::registerWindowFunction(
      name,
      std::move(signatures),
-      {exec::WindowFunction::ProcessMode::kRows, false},
+      {exec::WindowFunction::ProcessMode::kRows, false, false},


It doesn't seem right to allow specifying isCompanion for a window function.

Reverted, thanks.

mbasmanova · 2024-11-22T12:22:31Z

velox/functions/prestosql/aggregates/AverageAggregate.cpp

@@ -142,7 +142,7 @@ void registerAverageAggregate(
          }
        }
      },
-      {false /*orderSensitive*/},
+      {false /*orderSensitive*/, false /*isCompanionFunction*/},


It is strange to have an API that has both isCompanion and withCompanionFunctions

Yes it seems a bit strange, but isCompanionFunction is a part of the function metadata which indicates if the function being registered currently is a companion function, whereas withCompanionFunctions indicates if the aggregate should be registered with companion functions. So both these fields would be needed.
Could you please share how this could be made more readable?

mbasmanova · 2024-11-22T12:23:08Z

velox/functions/tests/FunctionRegistryTest.cpp

@@ -356,6 +357,23 @@ TEST_F(FunctionRegistryTest, isDeterministic) {
  ASSERT_FALSE(isDeterministic("not_found_function").has_value());
 }

+TEST_F(FunctionRegistryTest, isCompanionFunction) {
+  functions::prestosql::registerAllScalarFunctions();
+  // extract aggregate companion functions are registered as vector functions.


pramodsatya

Thanks for the feedback @mbasmanova. The companion function metadata added for window functions is now reverted and a check is added in registerAggregateFunction to ensure aggregate companion functions are no longer registered as window functions.
Could you please take another look?

pramodsatya · 2024-12-10T09:27:21Z

velox/functions/lib/window/RowNumber.cpp

@@ -75,7 +75,7 @@ void registerRowNumber(const std::string& name, TypeKind resultTypeKind) {
  exec::registerWindowFunction(
      name,
      std::move(signatures),
-      {exec::WindowFunction::ProcessMode::kRows, false},
+      {exec::WindowFunction::ProcessMode::kRows, false, false},


Reverted, thanks.

pramodsatya · 2024-12-10T09:41:48Z

velox/functions/prestosql/aggregates/AverageAggregate.cpp

@@ -142,7 +142,7 @@ void registerAverageAggregate(
          }
        }
      },
-      {false /*orderSensitive*/},
+      {false /*orderSensitive*/, false /*isCompanionFunction*/},


Yes it seems a bit strange, but isCompanionFunction is a part of the function metadata which indicates if the function being registered currently is a companion function, whereas withCompanionFunctions indicates if the aggregate should be registered with companion functions. So both these fields would be needed.
Could you please share how this could be made more readable?

aditi-pandit · 2025-01-15T18:21:56Z

@pedroerp : Pinged you about this review as well. Would you be able to help us to close this ? Appreciate it.

mbasmanova

Thanks.

aditi-pandit · 2025-01-16T17:17:26Z

Adding ready-to-merge label post Masha's review.

facebook-github-bot · 2025-01-29T17:35:34Z

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

bikramSingh91 · 2025-01-30T02:41:15Z

velox/functions/tests/FunctionRegistryTest.cpp

@@ -357,6 +358,23 @@ TEST_F(FunctionRegistryTest, isDeterministic) {
  ASSERT_FALSE(isDeterministic("not_found_function").has_value());
 }

+TEST_F(FunctionRegistryTest, companionFunction) {


This is failing internally with the following crash stacktrace:
Will dig into the issue and report back.

*** Aborted at 1738204790 (Unix time, try 'date -d @1738204790') *** *** Signal 6 (SIGABRT) (0x560a0007ff20) received by PID 524064 (pthread TID 0x7fd3b6f5bd40) (linux TID 524064) (maybe from PID 524064, UID 22026) (code: -6), stack trace: *** @ 000000000000f607 folly::symbolizer::(anonymous namespace)::innerSignalHandler(int, siginfo_t*, void*) ./fbcode/folly/debugging/symbolizer/SignalHandler.cpp:453 @ 000000000000dd81 folly::symbolizer::(anonymous namespace)::signalHandler(int, siginfo_t*, void*) ./fbcode/folly/debugging/symbolizer/SignalHandler.cpp:474 @ 000000000004455f (unknown) @ 000000000009c993 pthread_kill @ 00000000000444ac raise @ 000000000002c432 abort @ 000000000044e7bb std::__replacement_assert(char const*, int, char const*, char const*) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/x86_64-facebook-linux/bits/c++config.h:535 @ 00000000007271df std::_Optional_base_impl<facebook::velox::exec::VectorFunctionMetadata, std::_Optional_base<facebook::velox::exec::VectorFunctionMetadata, true, true> >::_M_get() fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/optional:440 @ 0000000000727144 std::optional<facebook::velox::exec::VectorFunctionMetadata>::operator->() fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/optional:919 @ 000000000042155f facebook::velox::FunctionRegistryTest_companionFunction_Test::TestBody() ./fbcode/velox/functions/tests/FunctionRegistryTest.cpp:371 @ 00000000001211be void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2675 @ 0000000000120a44 testing::Test::Run() fbsource/src/gtest.cc:2692 @ 000000000012668f testing::TestInfo::Run() fbsource/src/gtest.cc:2841 @ 000000000012e646 testing::TestSuite::Run() fbsource/src/gtest.cc:3020 @ 0000000000169fab testing::internal::UnitTestImpl::RunAllTests() fbsource/src/gtest.cc:5925 @ 000000000016900b bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) fbsource/src/gtest.cc:2675 @ 0000000000168549 testing::UnitTest::Run() fbsource/src/gtest.cc:5489 @ 0000000000002980 RUN_ALL_TESTS() fbsource/gtest/gtest.h:2317 -> ./fbcode/common/gtest/LightMain.cpp @ 000000000000260f main ./fbcode/common/gtest/LightMain.cpp:20

Sure, thanks @bikramSingh91.

looks like for the functions you used "array_frequency", "bitwise_left_shift", "ceil" are simple functions and are registered in another registry so exec::getVectorFunctionMetadata(function) returns a nullopt in this case which then crashes with a segfault when the test tries to deref it to access companionFunction.

I am not sure why its does not fail in the pre-commit tests, I also dont see velox_function_registry_test being run in the CI job. @kgpai any idea why that might be happening?

Can you check if this test runs on your local? wanna make sure that it fails

to fix this you can either use some vector functions instead or use the simple function registry to fetch their metadata, see (

velox/velox/functions/FunctionRegistry.cpp

Lines 80 to 82 in dcafd32

const auto simpleFunctions =

exec::simpleFunctions().getFunctionSignaturesAndMetadata(functionName);

const auto metadata = exec::getVectorFunctionMetadata(functionName);

) for reference.

Thanks for pointing it out, updated accordingly. Interestingly, the test runs locally both with exec::getVectorFunctionMetadata, and with the simple function registry lookup.

@pramodsatya Thanks for the update, main branch seems to be broken and the fix (#12223) is in works, do you mind waiting for it to merge and then rebase this change so we can get a clean CI.

@pramodsatya the build has been fixed, do you mind rebasing the PR so I can import it again? Thanks!

Thanks for the update @bikramSingh91, rebased the PR.

facebook-github-bot · 2025-02-03T17:43:33Z

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

facebook-github-bot · 2025-02-04T00:05:41Z

@bikramSingh91 merged this pull request in 2bb7aab.

pramodsatya requested review from czentgr and aditi-pandit March 26, 2024 05:17

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 26, 2024

pramodsatya mentioned this pull request Mar 26, 2024

[native] Retrieve Json function metadata for prestissimo functions prestodb/presto#22332

Merged

pramodsatya force-pushed the get_fn_metadata branch from bfea9ca to 3137659 Compare March 26, 2024 16:10

czentgr reviewed Mar 26, 2024

View reviewed changes

velox/functions/CoverageUtil.h Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.h Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.cpp Outdated Show resolved Hide resolved

aditi-pandit reviewed Mar 26, 2024

View reviewed changes

velox/functions/CoverageUtil.h Outdated Show resolved Hide resolved

velox/functions/CoverageUtil.h Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.cpp Outdated Show resolved Hide resolved

aditi-pandit changed the title ~~[WIP] Support retrieving function metadata~~ [WIP] Add FunctionRegistry APIs to retrieve function metadata. Mar 26, 2024

pramodsatya force-pushed the get_fn_metadata branch 3 times, most recently from c2011b1 to 8a28ec9 Compare March 29, 2024 02:49

pramodsatya requested review from czentgr and aditi-pandit March 29, 2024 02:50

pramodsatya force-pushed the get_fn_metadata branch from 8a28ec9 to 460e5db Compare May 30, 2024 18:52

pramodsatya changed the title ~~[WIP] Add FunctionRegistry APIs to retrieve function metadata.~~ Add FunctionRegistry APIs to retrieve function metadata. Jun 7, 2024

pramodsatya force-pushed the get_fn_metadata branch from 460e5db to 3eea7ab Compare June 7, 2024 16:11

aditi-pandit reviewed Jun 12, 2024

View reviewed changes

velox/functions/CMakeLists.txt Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.cpp Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.cpp Outdated Show resolved Hide resolved

pramodsatya force-pushed the get_fn_metadata branch from 3eea7ab to 29290d4 Compare August 1, 2024 23:10

pramodsatya commented Aug 1, 2024

View reviewed changes

velox/functions/CMakeLists.txt Outdated Show resolved Hide resolved

velox/functions/FunctionRegistry.cpp Outdated Show resolved Hide resolved

pramodsatya force-pushed the get_fn_metadata branch from 29290d4 to 8b822aa Compare August 2, 2024 00:31

pramodsatya marked this pull request as ready for review August 2, 2024 00:31

aditi-pandit reviewed Aug 2, 2024

View reviewed changes

pramodsatya force-pushed the get_fn_metadata branch from 8b822aa to 4f11b42 Compare August 2, 2024 02:26

pramodsatya changed the title ~~Add FunctionRegistry APIs to retrieve function metadata.~~ Expose helper functions to retrieve registered functions from FunctionRegistry Aug 3, 2024

pramodsatya force-pushed the get_fn_metadata branch from 4f11b42 to 3013349 Compare August 3, 2024 02:06

pramodsatya requested a review from aditi-pandit August 3, 2024 02:30

pramodsatya force-pushed the get_fn_metadata branch from 0b51694 to 0dccd1c Compare August 29, 2024 15:54

czentgr approved these changes Aug 29, 2024

View reviewed changes

czentgr reviewed Aug 29, 2024

View reviewed changes

mbasmanova reviewed Oct 24, 2024

View reviewed changes

pramodsatya force-pushed the get_fn_metadata branch from 0dccd1c to 64e6cc8 Compare October 25, 2024 18:31

pramodsatya requested a review from mbasmanova October 25, 2024 18:33

mbasmanova reviewed Nov 22, 2024

View reviewed changes

pramodsatya force-pushed the get_fn_metadata branch from 64e6cc8 to 6b91785 Compare December 10, 2024 10:04

pramodsatya commented Dec 10, 2024

View reviewed changes

pramodsatya changed the title ~~Add isCompanionFunction to function metadata~~ feat: Add companionFunction to function metadata Dec 10, 2024

aditi-pandit approved these changes Jan 15, 2025

View reviewed changes

aditi-pandit requested a review from pedroerp January 15, 2025 18:21

mbasmanova approved these changes Jan 16, 2025

View reviewed changes

aditi-pandit added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jan 16, 2025

pramodsatya force-pushed the get_fn_metadata branch from 6b91785 to 031c009 Compare January 28, 2025 17:55

bikramSingh91 reviewed Jan 30, 2025

View reviewed changes

pramodsatya force-pushed the get_fn_metadata branch from 031c009 to e060f27 Compare January 31, 2025 01:42

feat: Add companionFunction to function metadata

01de22d

pramodsatya force-pushed the get_fn_metadata branch from e060f27 to 01de22d Compare February 1, 2025 17:20

facebook-github-bot closed this in 2bb7aab Feb 4, 2025

facebook-github-bot added the Merged label Feb 4, 2025

	const auto simpleFunctions =
	exec::simpleFunctions().getFunctionSignaturesAndMetadata(functionName);
	const auto metadata = exec::getVectorFunctionMetadata(functionName);

feat: Add companionFunction to function metadata #9250

feat: Add companionFunction to function metadata #9250

Conversation

pramodsatya commented Mar 26, 2024 • edited Loading

netlify bot commented Mar 26, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

aditi-pandit commented Mar 26, 2024 • edited Loading

pramodsatya commented Mar 27, 2024 • edited Loading

pramodsatya commented Jun 7, 2024

pramodsatya left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit commented Aug 30, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

pramodsatya commented Oct 25, 2024

mbasmanova commented Nov 22, 2024

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

pramodsatya 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 commented Jan 15, 2025

mbasmanova left a comment

Choose a reason for hiding this comment

aditi-pandit commented Jan 16, 2025

facebook-github-bot commented Jan 29, 2025

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

bikramSingh91 Jan 31, 2025 • edited Loading

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 Feb 3, 2025

facebook-github-bot commented Feb 4, 2025

pramodsatya commented Mar 26, 2024 •

edited

Loading

netlify bot commented Mar 26, 2024 •

edited

Loading

aditi-pandit commented Mar 26, 2024 •

edited

Loading

pramodsatya commented Mar 27, 2024 •

edited

Loading

bikramSingh91 Jan 31, 2025 •

edited

Loading