Fix HyperLogLog type registration in registerApproxDistinctAggregates #10664

pramodsatya · 2024-08-05T17:27:38Z

Prefix is not needed to register custom type HyperLogLog in ApproxDistinctAggregates.
registerHyperLogLogType is used instead to make registration of customType HyperLogLog
uniform across velox.

Use of prefix in registerCustomType here resulted in the following error while running tests
in presto PR:

Reason: Type doesn't exist: 'HYPERLOGLOG'
Retriable: False
Function: validateBaseTypeAndCollectTypeParams
File: /Users/pramod/Desktop/velox/velox/expression/FunctionSignature.cpp
Line: 125

netlify · 2024-08-05T17:27:55Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`da531cf`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66b31c6fa091270008b81e3f

aditi-pandit · 2024-08-06T05:35:39Z

@pramodsatya : Your fix seems reasonable.

What did you do to encounter this error ? Please can you elaborate.

Reason: Type doesn't exist: 'HYPERLOGLOG'
Retriable: False
Function: validateBaseTypeAndCollectTypeParams
File: /Users/pramod/Desktop/velox/velox/expression/FunctionSignature.cpp
Line: 125

Its best if there is a Presto e2e query to illustrate this problem. We can then link the Presto PR to this PR to give the reviewers more details.

pramodsatya · 2024-08-06T05:56:25Z

Hi @aditi-pandit, I came across this issue while running the tests from FunctionMetadataTest.cpp in presto native function metadata PR. I observed this after making changes to address this review comment, to replace schema from default to whatever is configured in the presto namespace.

I ran the existing e2e tests for ApproxDistinctAggregate with this change and did not see any failures. I am not sure how it could be reproduced with a presto e2e test, since the hyperloglog custom type is already registered without a prefix in PrestoServer.cpp (from registerHyperLogFunctions() -> registerHyperLogLogType()). The check which is causing the failure, hasType(HYPERLOGLOG) -> customTypeExists(HYPERLOGLOG), would not fail in presto native and it seems like a bug in velox FunctionSignature. Could you please share how to proceed?

aditi-pandit · 2024-08-06T07:07:19Z

Hi @aditi-pandit, I came across this issue while running the tests from FunctionMetadataTest.cpp in presto native function metadata PR. I observed this after making changes to address this review comment, to replace schema from default to whatever is configured in the presto namespace.

I ran the existing e2e tests for ApproxDistinctAggregate with this change and did not see any failures. I am not sure how it could be reproduced with a presto e2e test, since the hyperloglog custom type is already registered without a prefix in PrestoServer.cpp (from registerHyperLogFunctions() -> registerHyperLogLogType()). The check which is causing the failure, hasType(HYPERLOGLOG) -> customTypeExists(HYPERLOGLOG), would not fail in presto native and it seems like a bug in velox FunctionSignature. Could you please share how to proceed?

@pramodsatya : It might be better to say that this code is changed to make it consistent with https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/types/HyperLogLogType.cpp#L20

Actually its even better if you use the registerHyperLogLogType() function itself instead of the registerCustomType call.

pramodsatya · 2024-08-06T17:52:28Z

Thanks for the suggestion @aditi-pandit, it is better to use registerHyperLogLogType here and updated the PR.

aditi-pandit · 2024-08-07T03:59:44Z

@pramodsatya : We should follow up with a PR adding e2e presto-native tests. Add a single function to test SQL with all the different functions using HLL to ensure the correctness of this fix.

aditi-pandit

Thanks @pramodsatya

mbasmanova

@pramodsatya Nice catch. The change looks good. Some comments re: test.

I assume this issue shows up only when we register aggregate, but not scalar functions. I see that scalar functions register 'hyperloglog' type correctly.

velox/exec/tests/FunctionSignatureBuilderTest.cpp

mbasmanova · 2024-08-07T04:56:46Z

velox/exec/tests/FunctionSignatureBuilderTest.cpp

+class SignaturesWithCustomType : public ::testing::Test {};
+
+TEST_F(SignaturesWithCustomType, customType) {
+  aggregate::prestosql::registerAllAggregateFunctions("presto.default.");


Let's use registerApproxDistinctAggregates

This is not present in any header and looks like only registerAllAggregateFunctions is exposed for tests. Is it fine if registerAllAggregateFunctions is retained?

velox/exec/tests/FunctionSignatureBuilderTest.cpp

pramodsatya

Thanks @mbasmanova, yes this issue shows up when we register aggregate functions with a prefix and do not register hyperloglog as a custom type (using registerHyperLogLogType).

pramodsatya · 2024-08-07T06:26:53Z

velox/exec/tests/FunctionSignatureBuilderTest.cpp

+class SignaturesWithCustomType : public ::testing::Test {};
+
+TEST_F(SignaturesWithCustomType, customType) {
+  aggregate::prestosql::registerAllAggregateFunctions("presto.default.");


This is not present in any header and looks like only registerAllAggregateFunctions is exposed for tests. Is it fine if registerAllAggregateFunctions is retained?

mbasmanova · 2024-08-07T06:55:54Z

velox/functions/prestosql/aggregates/tests/ApproxDistinctTest.cpp

@@ -432,5 +433,14 @@ TEST_F(ApproxDistinctTest, toIntermediate) {
      digests, {"c0"}, {"merge(a0)"}, {"c0", "cardinality(a0)"}, {input});
 }

+TEST(ApproxDistinctSignatureTest, hyperLogLog) {


As mentioned earlier, test name should match file name. Please, change to TEST_F(ApproxDistinctTest, ...

The test name 'hyperLogLog' is not very descriptive. Consider, typeRegistration.

It could be that changing to ApproxDistinctTest will end up registering scalar functions in the SetUp, in which case, this test won't reproduce the problem. I'm thinking that, perhaps, we don't need a test for this fix at all. Seems more trouble then worth it.

Yes actually I had changed it because when I used the file name as the test name I got this error when running the test:

velox/cmake-build-debug/_deps/gtest-src/googletest/src/gtest.cc:2520: Failure Failed All tests in the same test suite must use the same test fixture class, so mixing TEST_F and TEST in the same test suite is illegal. In test suite ApproxDistinctTest, test groupByIntegers is defined using TEST_F but test hyperLogLog is defined using TEST. You probably want to change the TEST to TEST_F or move it to another test case.

Apologies for not mentioning this earlier in the comment.

Yes, I agree a test doesn't seem suitable for this fix, reverted the test.

mbasmanova

@pramodsatya Thank you for the fix.

facebook-github-bot · 2024-08-12T22:12:06Z

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

facebook-github-bot · 2024-08-13T00:04:28Z

@xiaoxmeng merged this pull request in f340734.

conbench-facebook · 2024-08-13T00:30:37Z

Conbench analyzed the 1 benchmark run on commit f3407347.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

pramodsatya requested review from czentgr and aditi-pandit August 5, 2024 17:27

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

pramodsatya force-pushed the custom_type_fix branch from e7d42c3 to 2a37d91 Compare August 6, 2024 17:50

czentgr marked this pull request as ready for review August 6, 2024 20:56

aditi-pandit changed the title ~~Remove prefix from HyperLogLog type registration~~ Make HyperLogLog type registration consistent across different functions Aug 7, 2024

aditi-pandit approved these changes Aug 7, 2024

View reviewed changes

aditi-pandit requested review from kagamiori and mbasmanova August 7, 2024 04:01

mbasmanova changed the title ~~Make HyperLogLog type registration consistent across different functions~~ Fix HyperLogLog type registration in registerApproxDistinctAggregates Aug 7, 2024

mbasmanova reviewed Aug 7, 2024

View reviewed changes

pramodsatya force-pushed the custom_type_fix branch from 2a37d91 to 0d6c00f Compare August 7, 2024 06:50

pramodsatya commented Aug 7, 2024

View reviewed changes

mbasmanova reviewed Aug 7, 2024

View reviewed changes

Remove prefix from HyperLogLog type registration

da531cf

pramodsatya force-pushed the custom_type_fix branch from 0d6c00f to da531cf Compare August 7, 2024 07:04

mbasmanova approved these changes Aug 7, 2024

View reviewed changes

mbasmanova 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 Aug 7, 2024

facebook-github-bot closed this in f340734 Aug 13, 2024

facebook-github-bot added the Merged label Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HyperLogLog type registration in registerApproxDistinctAggregates #10664

Fix HyperLogLog type registration in registerApproxDistinctAggregates #10664

pramodsatya commented Aug 5, 2024 •

edited

Loading

netlify bot commented Aug 5, 2024 •

edited

Loading

aditi-pandit commented Aug 6, 2024

pramodsatya commented Aug 6, 2024

aditi-pandit commented Aug 6, 2024

pramodsatya commented Aug 6, 2024

aditi-pandit commented Aug 7, 2024

aditi-pandit left a comment

mbasmanova left a comment

mbasmanova Aug 7, 2024

pramodsatya Aug 7, 2024

pramodsatya left a comment

pramodsatya Aug 7, 2024

mbasmanova Aug 7, 2024

pramodsatya Aug 7, 2024

mbasmanova left a comment

facebook-github-bot commented Aug 12, 2024

facebook-github-bot commented Aug 13, 2024

conbench-facebook bot commented Aug 13, 2024

Fix HyperLogLog type registration in registerApproxDistinctAggregates #10664

Fix HyperLogLog type registration in registerApproxDistinctAggregates #10664

Conversation

pramodsatya commented Aug 5, 2024 • edited Loading

netlify bot commented Aug 5, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

aditi-pandit commented Aug 6, 2024

pramodsatya commented Aug 6, 2024

aditi-pandit commented Aug 6, 2024

pramodsatya commented Aug 6, 2024

aditi-pandit commented Aug 7, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Aug 7, 2024

Choose a reason for hiding this comment

pramodsatya Aug 7, 2024

Choose a reason for hiding this comment

pramodsatya left a comment

Choose a reason for hiding this comment

pramodsatya Aug 7, 2024

Choose a reason for hiding this comment

mbasmanova Aug 7, 2024

Choose a reason for hiding this comment

pramodsatya Aug 7, 2024

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 12, 2024

facebook-github-bot commented Aug 13, 2024

conbench-facebook bot commented Aug 13, 2024

pramodsatya commented Aug 5, 2024 •

edited

Loading

netlify bot commented Aug 5, 2024 •

edited

Loading