You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/10/02 22:39:54 UTC
[PR] [multistage] clean up transform function type (draft) [pinot]
walterddr opened a new pull request, #11726:
URL: https://github.com/apache/pinot/pull/11726
registration of the transform function is not complete with `_` separate and camelCase naming.
- [x] making the list of transform function complete
- [ ] making sure there's a scalar function equivalent (or explicitly document when there's not)
- [ ] add unit test
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344476086
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
technically we dont need any of these. but we already have ComparisonFunctions in scalar with these names so added `lessThan` but not `notIn` b/c we dont have that
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344482512
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
This is not the correct way to decide whether to add them though. Eventually we need scalar function for all of them, but that is orthogonal to this problem.
Does postgreSQL support these functions? If so, we need to add them, if not, we should not add them
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344574540
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
So 2 options. Either we allow underscore on both scalar and transform. Or we don't allow them in neither.
Problem is that transform function in v1 allows underscore so for backward compatibility we need to register them. It is not about syntax.
For syntax sake we should not allow underscore format all together.
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344473637
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
lol good catch. :-)
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344464051
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
Do we need these aliases for auto-generated functions? If so, why skipping `LESS_THAN` and `NOT_IN`?
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344747807
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
discussed offline. will remove these in separate PR as we should not allow these function explicitly being called. They are not consider SqlFunction but SqlOperators to begin with
due to the fact that we do allow these already in release 1.0, we will use a separate PR to deprecate them explicitly with backward-incompat flag
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344747807
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
discussed offline. will remove these in separate PR as we should not allow these function explicitly being called. They are not consider SqlFunction but SqlOperators to begin with
due to the fact that we do allow these already, we will use a separate PR to deprecate them
--
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
Re: [PR] [multistage] clean up transform function type (draft) [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11726:
URL: https://github.com/apache/pinot/pull/11726#issuecomment-1743908015
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11726](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1df2c58) into [master](https://app.codecov.io/gh/apache/pinot/commit/82140f1177a36687e6a32e2e9571f5660b07fce0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (82140f1) will **decrease** coverage by `48.67%`.
> Report is 10 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11726 +/- ##
=============================================
- Coverage 63.08% 14.42% -48.67%
+ Complexity 1117 201 -916
=============================================
Files 2342 2342
Lines 125797 125881 +84
Branches 19336 19357 +21
=============================================
- Hits 79358 18153 -61205
- Misses 40785 106190 +65405
+ Partials 5654 1538 -4116
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.42% <0.00%> (-48.50%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.42% <0.00%> (-48.67%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.42% <0.00%> (-48.66%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.42% <0.00%> (-0.03%)` | :arrow_down: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pinot/sql/parsers/rewriter/CLPDecodeRewriter.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9DTFBEZWNvZGVSZXdyaXRlci5qYXZh) | `0.00% <0.00%> (-74.25%)` | :arrow_down: |
| [...transform/function/CLPDecodeTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ0xQRGVjb2RlVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-94.12%)` | :arrow_down: |
| [...r/transform/function/InIdSetTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSW5JZFNldFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-38.10%)` | :arrow_down: |
| [...core/query/executor/ServerQueryExecutorV1Impl.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `0.00% <0.00%> (-64.60%)` | :arrow_down: |
| [...r/transform/function/TransformFunctionFactory.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25GYWN0b3J5LmphdmE=) | `0.00% <0.00%> (-89.54%)` | :arrow_down: |
| [...e/pinot/common/function/TransformFunctionType.java](https://app.codecov.io/gh/apache/pinot/pull/11726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25UeXBlLmphdmE=) | `0.00% <0.00%> (-89.33%)` | :arrow_down: |
... and [1511 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11726/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #11726:
URL: https://github.com/apache/pinot/pull/11726
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344862495
##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -70,25 +70,27 @@ public enum TransformFunctionType {
// predicate functions
EQUALS("equals"),
- NOT_EQUALS("not_equals"),
- GREATER_THAN("greater_than"),
- GREATER_THAN_OR_EQUAL("greater_than_or_equal"),
+ NOT_EQUALS("not_equals", "notEquals"),
Review Comment:
see #11734
--
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
Re: [PR] [multistage] clean up transform function type [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11726:
URL: https://github.com/apache/pinot/pull/11726#discussion_r1344425902
##########
pinot-common/src/test/java/org/apache/pinot/common/function/FunctionDefinitionRegistryTest.java:
##########
@@ -47,10 +47,8 @@ public class FunctionDefinitionRegistryTest {
// Scalar function
"scalar",
// Functions without scalar function counterpart as of now
- // TODO: Revisit this list when we add scalar function for these functions
- "arraylength", "arrayaverage", "arraymin", "arraymax", "arraysum",
- "valuein", "mapvalue", "inidset", "lookup", "groovy",
- "timeconvert", "datetimeconvert", "jsonextractscalar", "jsonextractkey", "clpdecode",
+ "arraylength", "arrayaverage", "arraymin", "arraymax", "arraysum", "clpdecode", "groovy",
Review Comment:
other than the ignored list here, all other functions (including their alternative naming are registered in FunctionRegistry (e.g. has correspondent in ScalarFunction impl)
--
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