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/06/24 20:32:00 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5613: [QueryContext] Use QueryContext in functions
Jackie-Jiang opened a new pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613
## Description
Replace `BrokerRequest` classes (`AggregationInfo`, `TransformExpressionTree`) with QueryContext classes (`FunctionContext`, `ExpressionContext`) for aggregation and transform functions
This is the last PR of removing `BrokerRequest` classes from the query execution path. After this PR, the query engine will be independent of `BrokerRequest` and only access `QueryContext` classes, and all the redundant expression parsing will be saved.
Besides replacing `BrokerRequest` classes, also refactored `DistinctCountThetaSketchAggregationFunction` to use the `QueryContext` classes, which solves the TODO of standardizing the predicate strings and simplifies the predicate handling.
Minor bug-fixes:
- Fix HLL log2m override for SQL query and make it case insensitive (In `BaseBrokerRequestHandler`)
- Add special argument handling for `DistinctCountRawThetaSketch` (In `BrokerRequestToQueryContextConverter`)
- Always use single argument `*` for `COUNT` when converting `FunctionCallAstNode ` to `FunctionContext` (In `QueryContextConverterUtils`)
## Upgrade Notes
Because of the standardization of the `DistinctCountThetaSketch` predicate strings, please upgrade Broker before Server. The new Broker can handle both standard and non-standard predicate strings for backward-compatibility.
----------------------------------------------------------------
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 pull request #5613: [QueryContext] Use QueryContext in functions
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613#issuecomment-649096775
@mayankshriv Can you please review the change in `DistinctCountThetaSketchAggregationFunction`?
----------------------------------------------------------------
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 merged pull request #5613: [QueryContext] Use QueryContext in functions
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613
----------------------------------------------------------------
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 #5613: [QueryContext] Use QueryContext in functions
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613#discussion_r446370363
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java
##########
@@ -83,16 +73,69 @@
* <li> Required: Last expression is the one that will be evaluated to compute final result. </li>
* </ul>
*/
- public DistinctCountThetaSketchAggregationFunction(List<String> arguments)
+ public DistinctCountThetaSketchAggregationFunction(List<ExpressionContext> arguments)
throws SqlParseException {
- int numExpressions = arguments.size();
+ int numArguments = arguments.size();
+
+ // NOTE: This function expects at least 3 arguments: theta-sketch column, parameters, post-aggregation expression.
+ Preconditions.checkArgument(numArguments >= 3,
+ "DistinctCountThetaSketch expects at least three arguments (theta-sketch column, parameters, post-aggregation expression), got: ",
+ numArguments);
+
+ // Initialize the theta-sketch column
+ _thetaSketchColumn = arguments.get(0);
+ Preconditions.checkArgument(_thetaSketchColumn.getType() == ExpressionContext.Type.IDENTIFIER,
+ "First argument of DistinctCountThetaSketch must be identifier (theta-sketch column)");
+
+ // Initialize the theta-sketch parameters
+ ExpressionContext paramsExpression = arguments.get(1);
+ Preconditions.checkArgument(paramsExpression.getType() == ExpressionContext.Type.LITERAL,
+ "Second argument of DistinctCountThetaSketch must be literal (parameters)");
+ _thetaSketchParams = ThetaSketchParams.fromString(paramsExpression.getLiteral());
- // This function expects at least 3 arguments: Theta Sketch Column, Predicates & final aggregation expression.
- Preconditions.checkArgument(numExpressions >= 3, "DistinctCountThetaSketch expects at least three arguments, got: ",
- numExpressions);
+ // Initialize the theta-sketch set operation builder
+ _setOperationBuilder = getSetOperationBuilder();
- // Initialize all the internal state.
- init(arguments);
+ // Initialize the input expressions
Review comment:
With `init()` the member variables cannot be declared as `final`. IMO declaring them as `final` can enhance the readability because these variables should only be set once and should not be changed.
----------------------------------------------------------------
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] mayankshriv commented on a change in pull request #5613: [QueryContext] Use QueryContext in functions
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613#discussion_r446425356
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java
##########
@@ -83,16 +73,69 @@
* <li> Required: Last expression is the one that will be evaluated to compute final result. </li>
* </ul>
*/
- public DistinctCountThetaSketchAggregationFunction(List<String> arguments)
+ public DistinctCountThetaSketchAggregationFunction(List<ExpressionContext> arguments)
throws SqlParseException {
- int numExpressions = arguments.size();
+ int numArguments = arguments.size();
+
+ // NOTE: This function expects at least 3 arguments: theta-sketch column, parameters, post-aggregation expression.
+ Preconditions.checkArgument(numArguments >= 3,
+ "DistinctCountThetaSketch expects at least three arguments (theta-sketch column, parameters, post-aggregation expression), got: ",
+ numArguments);
+
+ // Initialize the theta-sketch column
+ _thetaSketchColumn = arguments.get(0);
+ Preconditions.checkArgument(_thetaSketchColumn.getType() == ExpressionContext.Type.IDENTIFIER,
+ "First argument of DistinctCountThetaSketch must be identifier (theta-sketch column)");
+
+ // Initialize the theta-sketch parameters
+ ExpressionContext paramsExpression = arguments.get(1);
+ Preconditions.checkArgument(paramsExpression.getType() == ExpressionContext.Type.LITERAL,
+ "Second argument of DistinctCountThetaSketch must be literal (parameters)");
+ _thetaSketchParams = ThetaSketchParams.fromString(paramsExpression.getLiteral());
- // This function expects at least 3 arguments: Theta Sketch Column, Predicates & final aggregation expression.
- Preconditions.checkArgument(numExpressions >= 3, "DistinctCountThetaSketch expects at least three arguments, got: ",
- numExpressions);
+ // Initialize the theta-sketch set operation builder
+ _setOperationBuilder = getSetOperationBuilder();
- // Initialize all the internal state.
- init(arguments);
+ // Initialize the input expressions
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.
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] mayankshriv commented on a change in pull request #5613: [QueryContext] Use QueryContext in functions
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5613:
URL: https://github.com/apache/incubator-pinot/pull/5613#discussion_r445968533
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java
##########
@@ -83,16 +73,69 @@
* <li> Required: Last expression is the one that will be evaluated to compute final result. </li>
* </ul>
*/
- public DistinctCountThetaSketchAggregationFunction(List<String> arguments)
+ public DistinctCountThetaSketchAggregationFunction(List<ExpressionContext> arguments)
throws SqlParseException {
- int numExpressions = arguments.size();
+ int numArguments = arguments.size();
+
+ // NOTE: This function expects at least 3 arguments: theta-sketch column, parameters, post-aggregation expression.
+ Preconditions.checkArgument(numArguments >= 3,
+ "DistinctCountThetaSketch expects at least three arguments (theta-sketch column, parameters, post-aggregation expression), got: ",
+ numArguments);
+
+ // Initialize the theta-sketch column
+ _thetaSketchColumn = arguments.get(0);
+ Preconditions.checkArgument(_thetaSketchColumn.getType() == ExpressionContext.Type.IDENTIFIER,
+ "First argument of DistinctCountThetaSketch must be identifier (theta-sketch column)");
+
+ // Initialize the theta-sketch parameters
+ ExpressionContext paramsExpression = arguments.get(1);
+ Preconditions.checkArgument(paramsExpression.getType() == ExpressionContext.Type.LITERAL,
+ "Second argument of DistinctCountThetaSketch must be literal (parameters)");
+ _thetaSketchParams = ThetaSketchParams.fromString(paramsExpression.getLiteral());
- // This function expects at least 3 arguments: Theta Sketch Column, Predicates & final aggregation expression.
- Preconditions.checkArgument(numExpressions >= 3, "DistinctCountThetaSketch expects at least three arguments, got: ",
- numExpressions);
+ // Initialize the theta-sketch set operation builder
+ _setOperationBuilder = getSetOperationBuilder();
- // Initialize all the internal state.
- init(arguments);
+ // Initialize the input expressions
Review comment:
what's the issue with calling init()?
----------------------------------------------------------------
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