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/04/18 17:33:00 UTC
[GitHub] [incubator-pinot] mayankshriv opened a new pull request #5275:
Issue 5261: Support AggregationFunctions with multiple arguments (contd).
mayankshriv opened a new pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275
This PR is a continuation of https://github.com/apache/incubator-pinot/pull/5259
to address the issue https://github.com/apache/incubator-pinot/issues/5261.
1. Added new field in request.thrift `aggregationFunctionArgs` as a list of String
arguments for the aggregation funciton.
- Could not use the existing `aggregationParams` as it is a Map, and functions with
variable arguments may not provide a name for the arg (to be used as key in Map).
- Maintain backward compatibility by first check for the new field, and fall back to
the existing one if it does not exist.
2. Ensure that all calls to the old AggregationInfo.getAggregationParams() is replaced
with backward compatible AgguregationFunctionUtils.getAggregationArgs().
3. Since most aggregation functions today have just one argument, added a separate api
AggregationFuncitonContext.getFirstArgument() as an optimization.
4. Cleaned up getColumnName() and getResultColumnName() api's in AggregationFunctionContext
class to not require the column name argument, as this is already stored in the
AggregationFunction.
5. Modified all tests to use aggregationFunctionArgs instead of aggregationParams.
TODO:
Remove the AggregationFunctionContext class as AggregationFunctions now store their arguments,
and this class no longer provides any additional value.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410989468
##########
File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -233,32 +232,32 @@ private AggregationInfo buildAggregationInfo(Function function) {
throw new Pql2CompilationException("Aggregation function expects non null argument");
}
- String argumentString;
+ List<String> args = new ArrayList<>(operands.size());
String functionName = function.getOperator();
if (functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
- argumentString = "*";
+ args = Collections.singletonList("*");
} else {
- Set<String> expressions = new HashSet<>();
- StringBuilder sb = new StringBuilder();
- int numOperands = operands.size();
- for (int i = 0; i < numOperands; i++) {
- Expression expression = operands.get(i);
- String columnExpression = getColumnExpression(expression);
- if (expressions.add(columnExpression)) {
- // deduplicate the columns
- if (i != 0) {
- sb.append(CompilerConstants.AGGREGATION_FUNCTION_ARG_SEPARATOR);
+ // Need to de-dup columns for distinct.
+ if (functionName.equalsIgnoreCase(AggregationFunctionType.DISTINCT.getName())) {
+ Set<String> expressionSet = new HashSet<>();
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410980080
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/AggregationFunctionContext.java
##########
@@ -18,19 +18,28 @@
*/
package org.apache.pinot.core.query.aggregation;
+import com.google.common.base.Preconditions;
+import java.util.List;
import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
/**
* This class caches miscellaneous data to perform efficient aggregation.
+ *
+ * TODO: Remove this class, as it no longer provides any value after aggregation functions now store
Review comment:
+1
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410979361
##########
File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/PinotQuery2BrokerRequestConverter.java
##########
@@ -233,32 +232,32 @@ private AggregationInfo buildAggregationInfo(Function function) {
throw new Pql2CompilationException("Aggregation function expects non null argument");
}
- String argumentString;
+ List<String> args = new ArrayList<>(operands.size());
String functionName = function.getOperator();
if (functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
- argumentString = "*";
+ args = Collections.singletonList("*");
} else {
- Set<String> expressions = new HashSet<>();
- StringBuilder sb = new StringBuilder();
- int numOperands = operands.size();
- for (int i = 0; i < numOperands; i++) {
- Expression expression = operands.get(i);
- String columnExpression = getColumnExpression(expression);
- if (expressions.add(columnExpression)) {
- // deduplicate the columns
- if (i != 0) {
- sb.append(CompilerConstants.AGGREGATION_FUNCTION_ARG_SEPARATOR);
+ // Need to de-dup columns for distinct.
+ if (functionName.equalsIgnoreCase(AggregationFunctionType.DISTINCT.getName())) {
+ Set<String> expressionSet = new HashSet<>();
Review comment:
maintain the order?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410991343
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/AggregationFunctionContext.java
##########
@@ -18,19 +18,28 @@
*/
package org.apache.pinot.core.query.aggregation;
+import com.google.common.base.Preconditions;
+import java.util.List;
import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
/**
* This class caches miscellaneous data to perform efficient aggregation.
+ *
+ * TODO: Remove this class, as it no longer provides any value after aggregation functions now store
Review comment:
Seemed like a bit non-trivial, will address in the next PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410980204
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/AggregationFunctionContext.java
##########
@@ -18,19 +18,28 @@
*/
package org.apache.pinot.core.query.aggregation;
+import com.google.common.base.Preconditions;
+import java.util.List;
import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
/**
* This class caches miscellaneous data to perform efficient aggregation.
+ *
+ * TODO: Remove this class, as it no longer provides any value after aggregation functions now store
Review comment:
why not remove it in this PR itself?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410979824
##########
File path: pinot-common/src/thrift/request.thrift
##########
@@ -102,6 +102,11 @@ struct AggregationInfo {
2: optional map<string,string> aggregationParams;
3: optional bool isInSelectList;
+ // Backward compatible change to allow aggregation functions to take multiple arguments.
+ // We could not reuse aggregationParams, as it requires argument name (as key), which may not be
+ // available for aggregation functions with variable arguments.
+ 4: optional list<string> aggregationFunctionArgs;
Review comment:
too long args/argList/params/paramList/operands
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410813599
##########
File path: pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java
##########
@@ -89,8 +89,7 @@ public static boolean validate(BrokerRequest br1, BrokerRequest br2, boolean ign
return false;
}
} else if (br2.getGroupBy() != null) {
- LOGGER.error("tGroupBy did not match, br1.getGroupBy() = null, br2.getGroupBy() = {}",
- br2.getGroupBy());
+ LOGGER.error("tGroupBy did not match, br1.getGroupBy() = null, br2.getGroupBy() = {}", br2.getGroupBy());
Review comment:
typo tGroupBy
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410984113
##########
File path: pinot-common/src/main/java/org/apache/pinot/parsers/utils/BrokerRequestComparisonUtils.java
##########
@@ -89,8 +89,7 @@ public static boolean validate(BrokerRequest br1, BrokerRequest br2, boolean ign
return false;
}
} else if (br2.getGroupBy() != null) {
- LOGGER.error("tGroupBy did not match, br1.getGroupBy() = null, br2.getGroupBy() = {}",
- br2.getGroupBy());
+ LOGGER.error("tGroupBy did not match, br1.getGroupBy() = null, br2.getGroupBy() = {}", br2.getGroupBy());
Review comment:
Was in the original code that's why I missed it. Will fix.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#5275: Issue 5261: Support AggregationFunctions with multiple arguments
(contd).
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5275: Issue 5261: Support AggregationFunctions with multiple arguments (contd).
URL: https://github.com/apache/incubator-pinot/pull/5275#discussion_r410989510
##########
File path: pinot-common/src/thrift/request.thrift
##########
@@ -102,6 +102,11 @@ struct AggregationInfo {
2: optional map<string,string> aggregationParams;
3: optional bool isInSelectList;
+ // Backward compatible change to allow aggregation functions to take multiple arguments.
+ // We could not reuse aggregationParams, as it requires argument name (as key), which may not be
+ // available for aggregation functions with variable arguments.
+ 4: optional list<string> aggregationFunctionArgs;
Review comment:
Replaced with `expressions` based on offline discussion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org