You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/01 22:04:44 UTC
[GitHub] [pinot] walterddr opened a new pull request, #9707: initial commit to support all scalar/transform/filter match
walterddr opened a new pull request, #9707:
URL: https://github.com/apache/pinot/pull/9707
- create placeholder function if those can be implemented in the future
- ignore ones that don't make sense post transform
- rewrite some aliases so that they can be understood by V2 engine as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9707: [multistage] initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9707:
URL: https://github.com/apache/pinot/pull/9707#discussion_r1012183017
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -147,4 +157,48 @@ public static FunctionInfo getFunctionInfo(String functionName, int numParameter
private static String canonicalize(String functionName) {
return StringUtils.remove(functionName, '_').toLowerCase();
}
+
+ /**
+ * Placeholders for scalar function, they register and represents the signature for transform and filter predicate
+ * so that v2 engine can understand and plan them correctly.
+ */
+ private static class PlaceholderScalarFunctions {
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType,
+ Object defaultValue) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static String jsonExtractKey(String jsonFieldName, String jsonPath) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long timeConvert(String timeConvertInput, String fromFormat, String toFormat) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long extract(String extractFormat, long timestamp) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ // CHECKSTYLE:OFF
+ // @formatter:off
Review Comment:
no need. have to fix alias anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9707: [multistage] initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9707:
URL: https://github.com/apache/pinot/pull/9707#discussion_r1012198244
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -79,6 +79,9 @@ private FunctionRegistry() {
}
}
}
+ for (Method method : PlaceholderScalarFunctions.class.getMethods()) {
Review Comment:
yup wilco
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9707: initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9707:
URL: https://github.com/apache/pinot/pull/9707#discussion_r1012139262
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -147,4 +157,48 @@ public static FunctionInfo getFunctionInfo(String functionName, int numParameter
private static String canonicalize(String functionName) {
return StringUtils.remove(functionName, '_').toLowerCase();
}
+
+ /**
+ * Placeholders for scalar function, they register and represents the signature for transform and filter predicate
+ * so that v2 engine can understand and plan them correctly.
+ */
+ private static class PlaceholderScalarFunctions {
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType,
+ Object defaultValue) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static String jsonExtractKey(String jsonFieldName, String jsonPath) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long timeConvert(String timeConvertInput, String fromFormat, String toFormat) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long extract(String extractFormat, long timestamp) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ // CHECKSTYLE:OFF
+ // @formatter:off
+ public static boolean text_contains(String text, String pattern) {
Review Comment:
Will calcite match the underscore? If so, other things can also break such as `json_extract_scalar`
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -25,7 +25,7 @@
public enum TransformFunctionType {
- // Aggregation functions for single-valued columns
+ // arithmetic functions for single-valued columns
Review Comment:
(format) Keep the comment capitalized to be consistent
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -147,4 +157,48 @@ public static FunctionInfo getFunctionInfo(String functionName, int numParameter
private static String canonicalize(String functionName) {
return StringUtils.remove(functionName, '_').toLowerCase();
}
+
+ /**
+ * Placeholders for scalar function, they register and represents the signature for transform and filter predicate
+ * so that v2 engine can understand and plan them correctly.
+ */
+ private static class PlaceholderScalarFunctions {
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType,
+ Object defaultValue) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static String jsonExtractKey(String jsonFieldName, String jsonPath) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long timeConvert(String timeConvertInput, String fromFormat, String toFormat) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long extract(String extractFormat, long timestamp) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ // CHECKSTYLE:OFF
+ // @formatter:off
Review Comment:
Why do we need to turn them on?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9707: [multistage] initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9707:
URL: https://github.com/apache/pinot/pull/9707#discussion_r1012182086
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -147,4 +157,48 @@ public static FunctionInfo getFunctionInfo(String functionName, int numParameter
private static String canonicalize(String functionName) {
return StringUtils.remove(functionName, '_').toLowerCase();
}
+
+ /**
+ * Placeholders for scalar function, they register and represents the signature for transform and filter predicate
+ * so that v2 engine can understand and plan them correctly.
+ */
+ private static class PlaceholderScalarFunctions {
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static Object jsonExtractScalar(String jsonFieldName, String jsonPath, String resultsType,
+ Object defaultValue) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static String jsonExtractKey(String jsonFieldName, String jsonPath) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long timeConvert(String timeConvertInput, String fromFormat, String toFormat) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ public static long extract(String extractFormat, long timestamp) {
+ throw new UnsupportedOperationException("Placeholder scalar function, should not reach here");
+ }
+
+ // CHECKSTYLE:OFF
+ // @formatter:off
+ public static boolean text_contains(String text, String pattern) {
Review Comment:
+1. will fix this by adding annotation but only register to calcite map
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #9707: initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9707:
URL: https://github.com/apache/pinot/pull/9707#issuecomment-1299385769
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9707?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9707](https://codecov.io/gh/apache/pinot/pull/9707?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae7a2ca) into [master](https://codecov.io/gh/apache/pinot/commit/585e0448d47939e6984676e44db808f6e531d671?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (585e044) will **increase** coverage by `39.30%`.
> The diff coverage is `62.50%`.
```diff
@@ Coverage Diff @@
## master #9707 +/- ##
=============================================
+ Coverage 28.11% 67.41% +39.30%
- Complexity 53 5123 +5070
=============================================
Files 1934 1447 -487
Lines 103937 75664 -28273
Branches 15768 12056 -3712
=============================================
+ Hits 29221 51011 +21790
+ Misses 71858 20986 -50872
- Partials 2858 3667 +809
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.41% <62.50%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ot/common/function/scalar/ArithmeticFunctions.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0FyaXRobWV0aWNGdW5jdGlvbnMuamF2YQ==) | `62.50% <ø> (+41.66%)` | :arrow_up: |
| [...ot/common/function/scalar/ComparisonFunctions.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0NvbXBhcmlzb25GdW5jdGlvbnMuamF2YQ==) | `25.00% <ø> (-25.00%)` | :arrow_down: |
| [.../pinot/common/function/scalar/ObjectFunctions.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL09iamVjdEZ1bmN0aW9ucy5qYXZh) | `66.66% <ø> (+66.66%)` | :arrow_up: |
| [.../pinot/common/function/scalar/StringFunctions.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL1N0cmluZ0Z1bmN0aW9ucy5qYXZh) | `53.57% <ø> (+18.75%)` | :arrow_up: |
| [...apache/pinot/common/function/FunctionRegistry.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25SZWdpc3RyeS5qYXZh) | `75.47% <50.00%> (-11.38%)` | :arrow_down: |
| [...e/pinot/common/function/TransformFunctionType.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25UeXBlLmphdmE=) | `100.00% <100.00%> (+0.94%)` | :arrow_up: |
| [...inot/common/function/scalar/DateTimeFunctions.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `95.71% <100.00%> (+89.46%)` | :arrow_up: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1703 more](https://codecov.io/gh/apache/pinot/pull/9707/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9707: [multistage] initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9707:
URL: https://github.com/apache/pinot/pull/9707#discussion_r1012195939
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionRegistry.java:
##########
@@ -79,6 +79,9 @@ private FunctionRegistry() {
}
}
}
+ for (Method method : PlaceholderScalarFunctions.class.getMethods()) {
Review Comment:
thanks for this PR! can you add some test cases to `QueryRunnerTest` to validate that functions registered in this way are picked up end-to-end?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia merged pull request #9707: [multistage] initial commit to support all scalar/transform/filter match
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9707:
URL: https://github.com/apache/pinot/pull/9707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org