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] add calcite function catalog #9375

Merged
merged 6 commits into from
Sep 12, 2022

Conversation

walterddr
Copy link
Contributor

@walterddr walterddr commented Sep 10, 2022

Description

FunctionRegistry currently hosts lots of scalar functions. there are not registered with the Calcite catalog reader.

this PR registers

  • the functions with Calcite function catalog and
  • chained SqlOperator table with user-defined functions
  • clean up some bugs

TODO

  • support multiple functions overlead with the same number of arguments, but different parameter type.
  • support type casting and implicit type hoisting correctly.
  • consolidate number-based lookup and operand types lookup

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2022

Codecov Report

Merging #9375 (0a5e656) into master (ff0a353) will increase coverage by 0.09%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master    #9375      +/-   ##
============================================
+ Coverage     69.70%   69.80%   +0.09%     
- Complexity     4786     5094     +308     
============================================
  Files          1885     1885              
  Lines        100377   100387      +10     
  Branches      15275    15276       +1     
============================================
+ Hits          69972    70079     +107     
+ Misses        25454    25357      -97     
  Partials       4951     4951              
Flag Coverage Δ
integration1 26.17% <31.25%> (+0.04%) ⬆️
integration2 24.80% <18.75%> (+0.10%) ⬆️
unittests1 66.97% <87.50%> (+0.01%) ⬆️
unittests2 15.35% <56.25%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...che/pinot/query/planner/logical/RexExpression.java 76.19% <ø> (-3.18%) ⬇️
...a/org/apache/pinot/query/catalog/PinotCatalog.java 50.00% <50.00%> (+6.25%) ⬆️
...apache/pinot/common/function/FunctionRegistry.java 86.84% <83.33%> (-0.66%) ⬇️
.../java/org/apache/pinot/query/QueryEnvironment.java 82.14% <100.00%> (+0.66%) ⬆️
...org/apache/pinot/query/context/PlannerContext.java 92.30% <100.00%> (ø)
...pinot/query/parser/CalciteRexExpressionParser.java 51.06% <100.00%> (+1.06%) ⬆️
...ache/pinot/query/planner/logical/StagePlanner.java 93.85% <100.00%> (ø)
...mmon/request/context/predicate/NotInPredicate.java 87.50% <0.00%> (-6.25%) ⬇️
...tream/kafka20/server/KafkaDataServerStartable.java 72.91% <0.00%> (-6.25%) ⬇️
.../predicate/NotEqualsPredicateEvaluatorFactory.java 74.03% <0.00%> (-5.77%) ⬇️
... and 28 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kishoreg
Copy link
Member

Does it handle the case if a function is overloaded?

@walterddr
Copy link
Contributor Author

walterddr commented Sep 10, 2022

Does it handle the case if a function is overloaded?

the limitation is still the same as current behavior

  • we can handle overload with different number of arguments.
  • we can't have 2 functions overloaded with the same number of arguments. (this is a limitation of FunctionRegistry, not Calcite function catalog)

@walterddr walterddr force-pushed the pr_add_function_catalog branch from fd59bfe to 9b95e89 Compare September 11, 2022 22:30
@@ -86,6 +96,11 @@ public static void init() {
*/
public static void registerFunction(Method method, boolean nullableParameters) {
registerFunction(method.getName(), method, nullableParameters);

// Calcite ScalarFunctionImpl doesn't allow customized named functions. TODO: fix me.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the fix here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will need to follow up items in #8597

@walterddr walterddr merged commit 987480b into apache:master Sep 12, 2022
@Jackie-Jiang Jackie-Jiang added the multi-stage Related to the multi-stage query engine label Sep 13, 2022
@walterddr walterddr deleted the pr_add_function_catalog branch December 6, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants