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/03/12 00:20:00 UTC

[GitHub] [pinot] Jackie-Jiang opened a new pull request #8341: Canonicalize the function name for SQL parser

Jackie-Jiang opened a new pull request #8341:
URL: https://github.com/apache/pinot/pull/8341


   Canonicalize the function name to be upper case without underscore when parsing the expression


-- 
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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825410980



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());

Review comment:
       It’s not really a case of it being a bottleneck but a death by 1000 cuts in this area, no single improvement will be transformative, but processing sql allocates a lot which leads to a lot of CPU time spent on GC, if not much latency impact




-- 
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 pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#issuecomment-1067367576


   @richardstartin Can you please take another look? I have to change several tests to accommodate with the lower case change, but that should be able to save a lot of unnecessary extra string allocations since we use lower case to canonicalize function names in all other places.


-- 
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 edited a comment on pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#issuecomment-1066036653


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?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 [#8341](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1e2305c) into [master](https://codecov.io/gh/apache/pinot/commit/91c2ebbf297c4bf3fecb5f98413e9f00e324e2dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c2ebb) will **increase** coverage by `34.84%`.
   > The diff coverage is `71.42%`.
   
   > :exclamation: Current head 1e2305c differs from pull request most recent head 21114e6. Consider uploading reports for the commit 21114e6 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/8341/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8341       +/-   ##
   =============================================
   + Coverage     27.62%   62.47%   +34.84%     
   - Complexity        0     4113     +4113     
   =============================================
     Files          1624     1624               
     Lines         85450    85442        -8     
     Branches      12882    12873        -9     
   =============================================
   + Hits          23604    53378    +29774     
   + Misses        59631    28132    -31499     
   - Partials       2215     3932     +1717     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `29.05% <51.70%> (?)` | |
   | integration2 | `?` | |
   | unittests1 | `66.90% <74.01%> (?)` | |
   
   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/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ry/optimizer/statement/JsonStatementOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L0pzb25TdGF0ZW1lbnRPcHRpbWl6ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `61.74% <50.00%> (-0.73%)` | :arrow_down: |
   | [...izer/statement/StringPredicateFilterOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L1N0cmluZ1ByZWRpY2F0ZUZpbHRlck9wdGltaXplci5qYXZh) | `88.63% <50.00%> (+6.28%)` | :arrow_up: |
   | [.../parsers/rewriter/PredicateComparisonRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9QcmVkaWNhdGVDb21wYXJpc29uUmV3cml0ZXIuamF2YQ==) | `83.72% <66.66%> (+22.35%)` | :arrow_up: |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `88.78% <66.66%> (+34.57%)` | :arrow_up: |
   | [...n/DistinctCountThetaSketchAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50VGhldGFTa2V0Y2hBZ2dyZWdhdGlvbkZ1bmN0aW9uLmphdmE=) | `63.96% <71.42%> (+38.96%)` | :arrow_up: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `87.70% <73.21%> (+23.11%)` | :arrow_up: |
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `87.50% <91.66%> (+31.86%)` | :arrow_up: |
   | [...pinot/sql/parsers/rewriter/SelectionsRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9TZWxlY3Rpb25zUmV3cml0ZXIuamF2YQ==) | `89.74% <91.66%> (+39.74%)` | :arrow_up: |
   | [...e/pinot/common/function/TransformFunctionType.java](https://codecov.io/gh/apache/pinot/pull/8341/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%> (+15.85%)` | :arrow_up: |
   | ... and [1077 more](https://codecov.io/gh/apache/pinot/pull/8341/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) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c2ebb...21114e6](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov-commenter edited a comment on pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#issuecomment-1066036653


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?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 [#8341](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4590be1) into [master](https://codecov.io/gh/apache/pinot/commit/262dc50e236ed2af25a0cf8c67658a48731ce573?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (262dc50) will **decrease** coverage by `41.90%`.
   > The diff coverage is `51.74%`.
   
   > :exclamation: Current head 4590be1 differs from pull request most recent head 7715942. Consider uploading reports for the commit 7715942 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/8341/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8341       +/-   ##
   =============================================
   - Coverage     70.83%   28.93%   -41.91%     
   =============================================
     Files          1636     1624       -12     
     Lines         85804    85449      -355     
     Branches      12920    12876       -44     
   =============================================
   - Hits          60779    24724    -36055     
   - Misses        20836    58444    +37608     
   + Partials       4189     2281     -1908     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `28.93% <51.74%> (-0.03%)` | :arrow_down: |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   
   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/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...inot/pql/parsers/pql2/ast/OutputColumnAstNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9PdXRwdXRDb2x1bW5Bc3ROb2RlLmphdmE=) | `58.53% <0.00%> (-36.59%)` | :arrow_down: |
   | [...he/pinot/sql/parsers/rewriter/OrdinalsUpdater.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9PcmRpbmFsc1VwZGF0ZXIuamF2YQ==) | `55.00% <0.00%> (-35.00%)` | :arrow_down: |
   | [...n/DistinctCountThetaSketchAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50VGhldGFTa2V0Y2hBZ2dyZWdhdGlvbkZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-65.33%)` | :arrow_down: |
   | [...ry/optimizer/statement/JsonStatementOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L0pzb25TdGF0ZW1lbnRPcHRpbWl6ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `54.20% <33.33%> (-35.52%)` | :arrow_down: |
   | [...pinot/sql/parsers/rewriter/SelectionsRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9TZWxlY3Rpb25zUmV3cml0ZXIuamF2YQ==) | `53.84% <37.50%> (-36.16%)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `61.74% <50.00%> (-10.09%)` | :arrow_down: |
   | [...izer/statement/StringPredicateFilterOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L1N0cmluZ1ByZWRpY2F0ZUZpbHRlck9wdGltaXplci5qYXZh) | `84.31% <50.00%> (-1.97%)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `62.87% <62.50%> (-25.32%)` | :arrow_down: |
   | [.../parsers/rewriter/PredicateComparisonRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9QcmVkaWNhdGVDb21wYXJpc29uUmV3cml0ZXIuamF2YQ==) | `60.46% <66.66%> (-23.63%)` | :arrow_down: |
   | ... and [1188 more](https://codecov.io/gh/apache/pinot/pull/8341/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) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [262dc50...7715942](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] Jackie-Jiang commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825375678



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -757,6 +757,23 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
     }
   }
 
+  private static final Map<String, FilterKind> CANONICAL_NAME_TO_FILTER_KIND_MAP = new HashMap<String, FilterKind>() {{
+    for (FilterKind filterKind : FilterKind.values()) {
+      put(StringUtils.remove(filterKind.name(), '_'), filterKind);
+    }
+  }};

Review comment:
       Good point, didn't know that.
   Here is an article explaining why this is an anti-pattern: https://blog.jooq.org/dont-be-clever-the-double-curly-braces-anti-pattern




-- 
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 merged pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #8341:
URL: https://github.com/apache/pinot/pull/8341


   


-- 
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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825270669



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -757,6 +757,23 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
     }
   }
 
+  private static final Map<String, FilterKind> CANONICAL_NAME_TO_FILTER_KIND_MAP = new HashMap<String, FilterKind>() {{
+    for (FilterKind filterKind : FilterKind.values()) {
+      put(StringUtils.remove(filterKind.name(), '_'), filterKind);
+    }
+  }};
+
+  /**
+   * Converts the function name into its canonical form, but keeps FilterKind.name() as is if the function name matches
+   * it. We need to keep the FilterKind.name() as is because we might need to read the FilterKind via
+   * FilterKind.valueOf().
+   */
+  private static String canonicalize(String functionName) {
+    String canonicalName = StringUtils.remove(functionName, '_').toUpperCase();
+    FilterKind filterKind = CANONICAL_NAME_TO_FILTER_KIND_MAP.get(canonicalName);
+    return filterKind != null ? filterKind.name() : canonicalName;

Review comment:
       Basically this lookup could work for more forms if the name - [upper case, lower case] x [camel case, snake case, pascal case, none] and unless the user writes something like jSoNExTrACTScALaR the resolution wouldn’t need to allocate.




-- 
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 edited a comment on pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#issuecomment-1066036653


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?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 [#8341](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7715942) into [master](https://codecov.io/gh/apache/pinot/commit/262dc50e236ed2af25a0cf8c67658a48731ce573?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (262dc50) will **decrease** coverage by `6.82%`.
   > The diff coverage is `68.53%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/8341/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #8341      +/-   ##
   ============================================
   - Coverage     70.83%   64.01%   -6.83%     
   + Complexity     4258     4254       -4     
   ============================================
     Files          1636     1591      -45     
     Lines         85804    83919    -1885     
     Branches      12920    12710     -210     
   ============================================
   - Hits          60779    53719    -7060     
   - Misses        20836    26325    +5489     
   + Partials       4189     3875     -314     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `66.89% <74.79%> (-0.05%)` | :arrow_down: |
   | unittests2 | `14.17% <4.19%> (+<0.01%)` | :arrow_up: |
   
   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/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ry/optimizer/statement/JsonStatementOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L0pzb25TdGF0ZW1lbnRPcHRpbWl6ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `23.65% <30.00%> (-48.17%)` | :arrow_down: |
   | [...izer/statement/StringPredicateFilterOptimizer.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvc3RhdGVtZW50L1N0cmluZ1ByZWRpY2F0ZUZpbHRlck9wdGltaXplci5qYXZh) | `84.31% <50.00%> (-1.97%)` | :arrow_down: |
   | [.../parsers/rewriter/PredicateComparisonRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9QcmVkaWNhdGVDb21wYXJpc29uUmV3cml0ZXIuamF2YQ==) | `83.72% <66.66%> (-0.37%)` | :arrow_down: |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `86.91% <66.66%> (-2.81%)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `86.77% <71.42%> (-1.42%)` | :arrow_down: |
   | [...n/DistinctCountThetaSketchAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50VGhldGFTa2V0Y2hBZ2dyZWdhdGlvbkZ1bmN0aW9uLmphdmE=) | `63.96% <71.42%> (-1.36%)` | :arrow_down: |
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `87.50% <91.66%> (-0.47%)` | :arrow_down: |
   | [...pinot/sql/parsers/rewriter/SelectionsRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9TZWxlY3Rpb25zUmV3cml0ZXIuamF2YQ==) | `89.74% <91.66%> (-0.26%)` | :arrow_down: |
   | [...e/pinot/common/function/TransformFunctionType.java](https://codecov.io/gh/apache/pinot/pull/8341/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%> (ø)` | |
   | ... and [394 more](https://codecov.io/gh/apache/pinot/pull/8341/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) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [262dc50...7715942](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825269775



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -757,6 +757,23 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
     }
   }
 
+  private static final Map<String, FilterKind> CANONICAL_NAME_TO_FILTER_KIND_MAP = new HashMap<String, FilterKind>() {{
+    for (FilterKind filterKind : FilterKind.values()) {
+      put(StringUtils.remove(filterKind.name(), '_'), filterKind);
+    }
+  }};

Review comment:
       This is widely considered an antipattern because it creates a new class for each map this is done for. Doing this in a static block or using a factory method doesn’t add much code but doesn’t increase the number of class files, classes to load etc.




-- 
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 #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#issuecomment-1066036653


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?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 [#8341](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7715942) into [master](https://codecov.io/gh/apache/pinot/commit/262dc50e236ed2af25a0cf8c67658a48731ce573?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (262dc50) will **decrease** coverage by `56.65%`.
   > The diff coverage is `4.19%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/8341/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8341       +/-   ##
   =============================================
   - Coverage     70.83%   14.17%   -56.66%     
   + Complexity     4258       81     -4177     
   =============================================
     Files          1636     1591       -45     
     Lines         85804    83919     -1885     
     Branches      12920    12710      -210     
   =============================================
   - Hits          60779    11897    -48882     
   - Misses        20836    71126    +50290     
   + Partials       4189      896     -3293     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `14.17% <4.19%> (+<0.01%)` | :arrow_up: |
   
   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/8341?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/common/function/TransformFunctionType.java](https://codecov.io/gh/apache/pinot/pull/8341/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=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `0.00% <0.00%> (-87.97%)` | :arrow_down: |
   | [...inot/pql/parsers/pql2/ast/OutputColumnAstNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9PdXRwdXRDb2x1bW5Bc3ROb2RlLmphdmE=) | `0.00% <0.00%> (-95.13%)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `0.00% <0.00%> (-88.20%)` | :arrow_down: |
   | [...pache/pinot/sql/parsers/rewriter/AliasApplier.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9BbGlhc0FwcGxpZXIuamF2YQ==) | `0.00% <0.00%> (-96.30%)` | :arrow_down: |
   | [.../NonAggregationGroupByToDistinctQueryRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9Ob25BZ2dyZWdhdGlvbkdyb3VwQnlUb0Rpc3RpbmN0UXVlcnlSZXdyaXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...he/pinot/sql/parsers/rewriter/OrdinalsUpdater.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9PcmRpbmFsc1VwZGF0ZXIuamF2YQ==) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
   | [.../parsers/rewriter/PredicateComparisonRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9QcmVkaWNhdGVDb21wYXJpc29uUmV3cml0ZXIuamF2YQ==) | `0.00% <0.00%> (-84.10%)` | :arrow_down: |
   | [...pinot/sql/parsers/rewriter/SelectionsRewriter.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9TZWxlY3Rpb25zUmV3cml0ZXIuamF2YQ==) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8341/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-89.72%)` | :arrow_down: |
   | ... and [1317 more](https://codecov.io/gh/apache/pinot/pull/8341/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) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [262dc50...7715942](https://codecov.io/gh/apache/pinot/pull/8341?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825269887



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());
         if (functionName.equals("ITEM") || functionName.equals("DOT")) {
           // Calcite parses path expression such as "data[0][1].a.b[0]" into a chain of ITEM and/or DOT
           // functions. Collapse this chain into an identifier.
           StringBuffer path = new StringBuffer();
-          compilePathExpression(functionName, functionNode, path);

Review comment:
       Can you get rid of this StringBuffer while you’re here?




-- 
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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825270115



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());

Review comment:
       The broker allocates a lot in this area, do you think it’s worth mapping from common forms to canonical form to save some string allocs and copies?




-- 
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 change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825415782



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());

Review comment:
       I understand the concern of allocating extra strings here, but canonicalizing the function name to lower case here won't make things worse because it can avoid the extra allocation in multiple places such as converting to lower case in `FunctionContext` and creating `AggregationFunction` and `TransformFunction`.
   To further optimize this, we could potentially put all supported aggregation/transform/SqlKind into a map, but not sure if it can really save much and worth maintaining the map and code complexity as the map size could be large if we put all these keys. I'd prefer keeping it this way for now and optimize it in a separate PR if necessary.




-- 
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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825423051



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());

Review comment:
       Let’s revisit this at a later date




-- 
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 change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825377867



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());
         if (functionName.equals("ITEM") || functionName.equals("DOT")) {
           // Calcite parses path expression such as "data[0][1].a.b[0]" into a chain of ITEM and/or DOT
           // functions. Collapse this chain into an identifier.
           StringBuffer path = new StringBuffer();
-          compilePathExpression(functionName, functionNode, path);

Review comment:
       Do you mean replacing it with `StringBuilder`? Done




-- 
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 change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825384348



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());

Review comment:
       It requires function information to maintain this map, it might also be hard to derive the camel case, snake case etc from the original function name. To reduce the non-necessary allocation, I think we can use lower case here as we use lower case to canonicalize the function name in other places. Then if we find this is the next bottleneck, we can consider adding this map then.




-- 
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] richardstartin commented on a change in pull request #8341: Canonicalize the function name for SQL parser

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #8341:
URL: https://github.com/apache/pinot/pull/8341#discussion_r825411034



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -708,12 +708,12 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
       case OTHER:
       case OTHER_FUNCTION:
       case DOT:
-        functionName = functionNode.getOperator().getName().toUpperCase();
+        functionName = canonicalize(functionNode.getOperator().getName());
         if (functionName.equals("ITEM") || functionName.equals("DOT")) {
           // Calcite parses path expression such as "data[0][1].a.b[0]" into a chain of ITEM and/or DOT
           // functions. Collapse this chain into an identifier.
           StringBuffer path = new StringBuffer();
-          compilePathExpression(functionName, functionNode, path);

Review comment:
       👍




-- 
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