Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multistage][feature] function registry and lookup method consolidation #12229

Closed

Conversation

walterddr
Copy link
Contributor

@walterddr walterddr commented Jan 5, 2024

Rong Rong added 4 commits January 4, 2024 07:55
1. FunctionRegistry keeps the old FUNCTION_INFO_MAP only
2. moved Calcite Catalog-based schema.Function registry to its own package; along with a SqlOperator based PinotOperatorTable
3. both CatalogReader and OperatorTable utilizes ground truth function from PinotFunctionRegistry --> will be default once deprecate FunctionRegistry
4. PinotFunctionRegistry provides argument-type based lookup via the same method SqlValidator utilize to lookup routine (and lookup operator overload)
5. clean up multi-stage engine side accordingly
@walterddr walterddr force-pushed the pr_function_reg_consolidation branch 2 times, most recently from 83d9fdc to 63b1002 Compare January 5, 2024 17:49
@walterddr walterddr changed the title [multistage][feature] function reg consolidation with lookup and deprecation removal [multistage][feature] function registry and consolidation Jan 5, 2024
@walterddr walterddr changed the title [multistage][feature] function registry and consolidation [multistage][feature] function registry and lookup method consolidation Jan 5, 2024
- use signature type lookup for v2 engine
- deprecate usage of FunctionRegistry
- allow nullable return from function lookup b/c some operators doesn't
  have scalar equivalent at the moment
@walterddr walterddr force-pushed the pr_function_reg_consolidation branch from 63b1002 to 65f6790 Compare January 5, 2024 18:10
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (d1cc17c) 61.53% compared to head (8b50e28) 61.39%.
Report is 3 commits behind head on master.

Files Patch % Lines
...apache/pinot/common/function/FunctionRegistry.java 80.72% 11 Missing and 5 partials ⚠️
.../pinot/common/function/sql/PinotOperatorTable.java 56.00% 9 Missing and 2 partials ⚠️
...ot/common/function/schema/PinotScalarFunction.java 73.33% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12229      +/-   ##
============================================
- Coverage     61.53%   61.39%   -0.14%     
- Complexity      207     1155     +948     
============================================
  Files          2416     2421       +5     
  Lines        131177   131487     +310     
  Branches      20245    20295      +50     
============================================
+ Hits          80717    80724       +7     
- Misses        44570    44847     +277     
- Partials       5890     5916      +26     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 27.63% <0.00%> (-33.86%) ⬇️
java-21 61.38% <77.69%> (-0.01%) ⬇️
skip-bytebuffers-false 61.38% <77.69%> (-0.15%) ⬇️
skip-bytebuffers-true 61.36% <77.69%> (-0.01%) ⬇️
temurin 61.39% <77.69%> (-0.14%) ⬇️
unittests 61.38% <77.69%> (-0.14%) ⬇️
unittests1 46.47% <77.69%> (-0.15%) ⬇️
unittests2 27.63% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- merge PinotFunctionRegistry with FunctionRegistry
- renamed to match calcite.schema and calcite.sql from
  pinot.common.function package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants