[poc][multistage][feature] function registry unification #12163

walterddr · 2023-12-15T18:05:01Z

working POC for a unified function registry. (extended version from #12144)

Design Doc: https://docs.google.com/document/d/1VcH8k6vU3dTdb717qJo1bgZAAIkeTso6KwKXkzpbAjk/edit
- Follow-up POC for unified polymorphism (type-lookup): [POC] function reg full use arg type lookup walterddr/pinot#92
- Follow-up POC for parameter-type-based lookup (v2 only): [POC] function reg full transform scalar merge walterddr/pinot#91

codecov-commenter · 2023-12-18T17:11:15Z

Codecov Report

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

Comparison is base (d1cc17c) 61.53% compared to head (89c8335) 61.52%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
...not/common/function/sql/PinotFunctionRegistry.java	58.13%	32 Missing and 4 partials ⚠️
.../pinot/common/function/sql/PinotOperatorTable.java	56.00%	9 Missing and 2 partials ⚠️
...apache/pinot/common/function/FunctionRegistry.java	85.71%	1 Missing and 3 partials ⚠️
.../common/function/registry/PinotScalarFunction.java	73.33%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12163      +/-   ##
============================================
- Coverage     61.53%   61.52%   -0.01%     
- Complexity      207     1152     +945     
============================================
  Files          2416     2422       +6     
  Lines        131177   131512     +335     
  Branches      20245    20302      +57     
============================================
+ Hits          80717    80912     +195     
- Misses        44570    44679     +109     
- Partials       5890     5921      +31

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	`61.46% <64.51%> (-0.03%)`	⬇️
java-21	`61.39% <64.51%> (-0.01%)`	⬇️
skip-bytebuffers-false	`61.48% <64.51%> (-0.04%)`	⬇️
skip-bytebuffers-true	`61.37% <64.51%> (-0.01%)`	⬇️
temurin	`61.52% <64.51%> (-0.01%)`	⬇️
unittests	`61.51% <64.51%> (-0.01%)`	⬇️
unittests1	`46.64% <64.51%> (+0.01%)`	⬆️
unittests2	`27.64% <0.00%> (-0.06%)`	⬇️

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.

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 · 2024-01-04T23:41:07Z

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java

@@ -38,19 +42,14 @@

 /**
 * Registry for scalar functions.
- * <p>TODO: Merge FunctionRegistry and FunctionDefinitionRegistry to provide one single registry for all functions.
 */
 public class FunctionRegistry {


Note to reviewer: this class will be maintain until

all Transform/Scalar functions are unified

all legacy, arg-count based function resolution logics are deprecated

walterddr · 2024-01-04T23:42:11Z

pinot-common/src/main/java/org/apache/pinot/common/function/sql/PinotFunctionRegistry.java

+    LOGGER.info("Initialized FunctionRegistry with {} functions: {} in {}ms", FUNCTION_MAP.map().size(),
+        FUNCTION_MAP.map().keySet(), System.currentTimeMillis() - startTimeMs);
+
+    // REGISTER OPERATORS


Note to reviewer:
this part will be maintain until all Transform function are registered in scalar form via PinotScalarFunction

walterddr · 2024-01-04T23:42:45Z

pinot-common/src/main/java/org/apache/pinot/common/function/sql/PinotFunctionRegistry.java

+    return argTypes.stream().map(type -> toRelType(typeFactory, type)).collect(Collectors.toList());
+  }
+
+  private static RelDataType toRelType(RelDataTypeFactory typeFactory, DataSchema.ColumnDataType dataType) {


Note to reviewer:
this and several other type conversion utils should be unified, we will follow up in separate PRs

walterddr · 2024-01-04T23:43:33Z

pinot-common/src/main/java/org/apache/pinot/common/function/sql/PinotOperatorTable.java

+
+
+/**
+ * Temporary implementation of all dynamic arg/return type inference operators.


Note to reviewer:
This will be maintained until all operators can be merged into catalog

walterddr · 2024-01-04T23:45:10Z

pinot-query-planner/src/main/java/org/apache/calcite/sql/util/PinotSqlStdOperatorTable.java

+import org.apache.pinot.common.function.FunctionRegistry;
+
+
+public class PinotSqlStdOperatorTable extends SqlStdOperatorTable {


Note to reviewer:
this class now only handles overries against SqlStdOperatorTable. it doesn't handle ANY Pinot special operators/functions

walterddr · 2024-01-05T19:02:44Z

closing in favor of #12229

walterddr force-pushed the poc_function_reg_full branch 2 times, most recently from f15c394 to 5f65e91 Compare December 18, 2023 17:04

walterddr added the feature label Dec 18, 2023

Rong Rong added 4 commits January 4, 2024 07:55

initial commit to make FunctionRegistry use Calcite functions

f913f82

adding back fallback option to current resolution

770dfd9

fix tests

7a9c87c

walterddr marked this pull request as ready for review January 4, 2024 23:00

walterddr changed the title ~~[POC] function registry unification~~ [multistage][feature] function registry unification Jan 4, 2024

walterddr commented Jan 4, 2024

View reviewed changes

walterddr force-pushed the poc_function_reg_full branch from 5f65e91 to 89c8335 Compare January 5, 2024 17:39

walterddr mentioned this pull request Jan 5, 2024

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

Closed

walterddr changed the title ~~[multistage][feature] function registry unification~~ [poc][multistage][feature] function registry unification Jan 5, 2024

walterddr marked this pull request as draft January 5, 2024 19:01

walterddr closed this Jan 5, 2024

walterddr mentioned this pull request Jan 22, 2024

[multistage][feature] FunctionRegistry unification #12302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[poc][multistage][feature] function registry unification #12163

[poc][multistage][feature] function registry unification #12163

walterddr commented Dec 15, 2023 •

edited

Loading

codecov-commenter commented Dec 18, 2023 •

edited

Loading

walterddr Jan 4, 2024

walterddr Jan 4, 2024

walterddr Jan 4, 2024

walterddr Jan 4, 2024 •

edited

Loading

walterddr Jan 4, 2024 •

edited

Loading

walterddr commented Jan 5, 2024



		/**
		* Temporary implementation of all dynamic arg/return type inference operators.

		import org.apache.pinot.common.function.FunctionRegistry;


		public class PinotSqlStdOperatorTable extends SqlStdOperatorTable {

[poc][multistage][feature] function registry unification #12163

[poc][multistage][feature] function registry unification #12163

Conversation

walterddr commented Dec 15, 2023 • edited Loading

codecov-commenter commented Dec 18, 2023 • edited Loading

Codecov Report

walterddr Jan 4, 2024

Choose a reason for hiding this comment

walterddr Jan 4, 2024

Choose a reason for hiding this comment

walterddr Jan 4, 2024

Choose a reason for hiding this comment

walterddr Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

walterddr Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

walterddr commented Jan 5, 2024

walterddr commented Dec 15, 2023 •

edited

Loading

codecov-commenter commented Dec 18, 2023 •

edited

Loading

walterddr Jan 4, 2024 •

edited

Loading

walterddr Jan 4, 2024 •

edited

Loading