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 2020/11/06 06:51:40 UTC

[GitHub] [incubator-pinot] fx19880617 opened a new pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

fx19880617 opened a new pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246


   ##  Description
   Adding validation for json_extract_key and json_extract_scalar functions during sql compilation phase to avoid empty response
   
   If user mistakenly put double quote on a the json key argument, say `jsonExtractScalar(myMapStr,"$.k1", 'STRING')`, then query parser recognizes the second argument `$.k1` as an identifier.  Then during segment pruning phase, the DataSchemaSegmentPruner prunes all the segments as there is no column matches `$.k1`. Then user will get an empty results.
   
   Adding a function validation method during compilation time will allow us to find out the expression type mismatch then error out earlier and direct more comprehensive errors and guides to users.


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

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] [incubator-pinot] codecov-io commented on pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246#issuecomment-722928327


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=h1) Report
   > Merging [#6246](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `2.32%`.
   > The diff coverage is `43.43%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6246/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6246      +/-   ##
   ==========================================
   - Coverage   66.44%   64.11%   -2.33%     
   ==========================================
     Files        1075     1240     +165     
     Lines       54773    58954    +4181     
     Branches     8168     8739     +571     
   ==========================================
   + Hits        36396    37801    +1405     
   - Misses      15700    18398    +2698     
   - Partials     2677     2755      +78     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.11% <43.43%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | ... and [1078 more](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=footer). Last update [d19d604...1ba42e8](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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] [incubator-pinot] codecov-io edited a comment on pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246#issuecomment-722928327


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=h1) Report
   > Merging [#6246](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `2.34%`.
   > The diff coverage is `43.43%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6246/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6246      +/-   ##
   ==========================================
   - Coverage   66.44%   64.10%   -2.35%     
   ==========================================
     Files        1075     1240     +165     
     Lines       54773    58954    +4181     
     Branches     8168     8739     +571     
   ==========================================
   + Hits        36396    37793    +1397     
   - Misses      15700    18410    +2710     
   - Partials     2677     2751      +74     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `64.10% <43.43%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
   | [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | ... and [1077 more](https://codecov.io/gh/apache/incubator-pinot/pull/6246/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=footer). Last update [d19d604...53807b8](https://codecov.io/gh/apache/incubator-pinot/pull/6246?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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] [incubator-pinot] fx19880617 edited a comment on pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

Posted by GitBox <gi...@apache.org>.
fx19880617 edited a comment on pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246#issuecomment-777379824


   > Why do we have this validation inside the parser? We should have a query validation phase after the query is parsed
   
   The reason is that the current stack trace is very confusing and has no way for users to figure out what's wrong. Those identifiers and literals check will be much useful to help users correct their queries. 
   
   Please suggest if you feel there is a better place to put 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.

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] [incubator-pinot] fx19880617 commented on pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246#issuecomment-777379824


   > Why do we have this validation inside the parser? We should have a query validation phase after the query is parsed
   
   The reason is that the current stack trace is very confusing and has no way for users to figure out what's wrong. Those identifiers and literals check will be much useful to help users correct their queries. 
   
   Please let me know if you find some better place to put 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.

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] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

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



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {
+      case "JSONEXTRACTSCALAR":
+        validateJsonExtractScalarFunction(functionName, operands);
+        break;
+      case "JSONEXTRACTKEY":
+        validateJsonExtractKeyFunction(functionName, operands);
+        break;
+    }
+  }
+
+  private static void validateJsonExtractScalarFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() < 3 || operands.size() > 4) {
+      throw new SqlCompilationException(
+          "Expected 3/4 arguments for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue'])");
+    }
+    if (!operands.get(1).isSetLiteral() || !operands.get(2).isSetLiteral()) {

Review comment:
       Also validate the optional fourth argument?

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {
+      case "JSONEXTRACTSCALAR":
+        validateJsonExtractScalarFunction(functionName, operands);
+        break;
+      case "JSONEXTRACTKEY":
+        validateJsonExtractKeyFunction(functionName, operands);
+        break;
+    }
+  }
+
+  private static void validateJsonExtractScalarFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() < 3 || operands.size() > 4) {

Review comment:
       (nit) For clarity
   ```suggestion
       if (operands.size() != 3 && operands.size() != 4) {
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {
+      case "JSONEXTRACTSCALAR":
+        validateJsonExtractScalarFunction(functionName, operands);
+        break;
+      case "JSONEXTRACTKEY":
+        validateJsonExtractKeyFunction(functionName, operands);
+        break;
+    }
+  }
+
+  private static void validateJsonExtractScalarFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() < 3 || operands.size() > 4) {
+      throw new SqlCompilationException(
+          "Expected 3/4 arguments for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue'])");
+    }
+    if (!operands.get(1).isSetLiteral() || !operands.get(2).isSetLiteral()) {
+      throw new SqlCompilationException(
+          "Expected the second or third argument for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue']) to be a single-quoted literal value.");
+    }
+  }
+
+  private static void validateJsonExtractKeyFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments

Review comment:
       Update the comment

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {
+      case "JSONEXTRACTSCALAR":
+        validateJsonExtractScalarFunction(functionName, operands);
+        break;
+      case "JSONEXTRACTKEY":
+        validateJsonExtractKeyFunction(functionName, operands);
+        break;
+    }
+  }
+
+  private static void validateJsonExtractScalarFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() < 3 || operands.size() > 4) {
+      throw new SqlCompilationException(
+          "Expected 3/4 arguments for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue'])");

Review comment:
       `"Expect ..."`, same for other exception message

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {
+      case "JSONEXTRACTSCALAR":
+        validateJsonExtractScalarFunction(functionName, operands);
+        break;
+      case "JSONEXTRACTKEY":
+        validateJsonExtractKeyFunction(functionName, operands);
+        break;
+    }
+  }
+
+  private static void validateJsonExtractScalarFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() < 3 || operands.size() > 4) {
+      throw new SqlCompilationException(
+          "Expected 3/4 arguments for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue'])");
+    }
+    if (!operands.get(1).isSetLiteral() || !operands.get(2).isSetLiteral()) {
+      throw new SqlCompilationException(
+          "Expected the second or third argument for transform function: jsonExtractScalar(jsonFieldName, 'jsonPath', 'resultsType', ['defaultValue']) to be a single-quoted literal value.");
+    }
+  }
+
+  private static void validateJsonExtractKeyFunction(String functionName, List<Expression> operands) {
+    // Check that there are exactly 3 or 4 arguments
+    if (operands.size() != 2) {
+      throw new SqlCompilationException(
+          "Exactly 2 arguments are required for transform function: jsonExtractKey(jsonFieldName, 'jsonPath')");
+    }
+    if (!operands.get(1).isSetLiteral() || !operands.get(2).isSetLiteral()) {

Review comment:
       (Critical)
   ```suggestion
       if (!operands.get(1).isSetLiteral()) {
   ```

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -902,11 +902,47 @@ private static Expression compileFunctionExpression(SqlBasicCall functionNode) {
         operands.add(toExpression(childNode));
       }
     }
+    validateFunction(functionName, operands);
     Expression functionExpression = RequestUtils.getFunctionExpression(functionName);
     functionExpression.getFunctionCall().setOperands(operands);
     return functionExpression;
   }
 
+  private static void validateFunction(String functionName, List<Expression> operands) {
+    switch (functionName.toUpperCase()) {

Review comment:
       Remove underscore from the function name to match `json_extract_scalar` and `json_extract_key`. Please also add some test cases for function name with underscore




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

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] [incubator-pinot] fx19880617 commented on pull request #6246: Adding validation for json_extract_key and json_extract_scalar functions

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246#issuecomment-777773840


   > Why do we have this validation inside the parser? We should have a query validation phase after the query is parsed
   
   Also it allows the query to fail fast at controller/broker not scatter-gather to servers.


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

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] [incubator-pinot] fx19880617 merged pull request #6246: Adding validation for jsonExtractKey and jsonExtractScalar functions

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #6246:
URL: https://github.com/apache/incubator-pinot/pull/6246


   


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

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