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/08/03 20:24:34 UTC
[GitHub] [incubator-pinot] fx19880617 opened a new pull request #5795: Support aggregation function name with underscore inside
fx19880617 opened a new pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795
## Description
Support aggregation function name with underscore inside
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r465385004
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/AggregationFunctionType.java
##########
@@ -101,6 +101,13 @@ public static AggregationFunctionType getAggregationFunctionType(String function
try {
return AggregationFunctionType.valueOf(upperCaseFunctionName);
} catch (Exception e) {
+ if (upperCaseFunctionName.contains("_")) {
Review comment:
This won't cover functions such as `percentile_tdigest`.
We can make the current one a private helper method, and add a new `getAggregationFunctionType` to look up for both functionName with/without underscore
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java
##########
@@ -106,7 +106,7 @@ public static AggregationFunction getAggregationFunction(FunctionContext functio
}
throw new IllegalArgumentException("Invalid percentile function: " + function);
} else {
- switch (AggregationFunctionType.valueOf(upperCaseFunctionName)) {
+ switch (AggregationFunctionType.getAggregationFunctionType(upperCaseFunctionName)) {
Review comment:
Same 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.
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#issuecomment-669699817
> I think the current version works, but has some inefficiency.
> Should we just change `ST_UNION` to `STUNION` and always remove underscore if exists? The logic will be much simpler. Sorry to go back and forth on this.
I actually prefer to this way before we have two functions with same name after removing all "_" :)
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r466082450
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java
##########
@@ -45,10 +45,12 @@ private AggregationFunctionFactory() {
public static AggregationFunction getAggregationFunction(FunctionContext function, QueryContext queryContext) {
try {
String upperCaseFunctionName = function.getFunctionName().toUpperCase();
+ AggregationFunctionType aggregationFunctionType =
+ AggregationFunctionType.getAggregationFunctionType(upperCaseFunctionName);
List<ExpressionContext> arguments = function.getArguments();
ExpressionContext firstArgument = arguments.get(0);
- if (upperCaseFunctionName.startsWith("PERCENTILE")) {
- String remainingFunctionName = upperCaseFunctionName.substring(10);
+ if (aggregationFunctionType.getName().toUpperCase().startsWith("PERCENTILE")) {
Review comment:
`.name()` will return upper case though :-P
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#issuecomment-669620085
I think the current version works, but has some inefficiency.
Should we just change `ST_UNION` to `STUNION` and always remove underscore if exists? The logic will be much simpler. Sorry to go back and forth on this.
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r464692098
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/AggregationFunctionType.java
##########
@@ -79,7 +79,7 @@ public boolean isOfType(AggregationFunctionType... aggregationFunctionTypes) {
* Returns the corresponding aggregation function type for the given function name.
*/
public static AggregationFunctionType getAggregationFunctionType(String functionName) {
- String upperCaseFunctionName = functionName.toUpperCase();
+ String upperCaseFunctionName = functionName.toUpperCase().replace("_", "");
Review comment:
This will break `ST_Union`. Can we first look up with original function name, and if not found, replace `_` 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.
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] siddharthteotia commented on a change in pull request #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r466062189
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java
##########
@@ -45,10 +45,12 @@ private AggregationFunctionFactory() {
public static AggregationFunction getAggregationFunction(FunctionContext function, QueryContext queryContext) {
try {
String upperCaseFunctionName = function.getFunctionName().toUpperCase();
+ AggregationFunctionType aggregationFunctionType =
+ AggregationFunctionType.getAggregationFunctionType(upperCaseFunctionName);
List<ExpressionContext> arguments = function.getArguments();
ExpressionContext firstArgument = arguments.get(0);
- if (upperCaseFunctionName.startsWith("PERCENTILE")) {
- String remainingFunctionName = upperCaseFunctionName.substring(10);
+ if (aggregationFunctionType.getName().toUpperCase().startsWith("PERCENTILE")) {
Review comment:
Why do we have to do upperCase again?
AggregationFunctionType is an enum so getName() should return upper case right?
----------------------------------------------------------------
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 #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r465386143
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/AggregationFunctionType.java
##########
@@ -101,6 +101,13 @@ public static AggregationFunctionType getAggregationFunctionType(String function
try {
return AggregationFunctionType.valueOf(upperCaseFunctionName);
} catch (Exception e) {
+ if (upperCaseFunctionName.contains("_")) {
+ try {
+ return AggregationFunctionType.valueOf(upperCaseFunctionName.replace("_", ""));
Review comment:
Check if it contains underscore before the second lookup for performance
----------------------------------------------------------------
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 a change in pull request #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r466071366
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java
##########
@@ -45,10 +45,12 @@ private AggregationFunctionFactory() {
public static AggregationFunction getAggregationFunction(FunctionContext function, QueryContext queryContext) {
try {
String upperCaseFunctionName = function.getFunctionName().toUpperCase();
+ AggregationFunctionType aggregationFunctionType =
+ AggregationFunctionType.getAggregationFunctionType(upperCaseFunctionName);
List<ExpressionContext> arguments = function.getArguments();
ExpressionContext firstArgument = arguments.get(0);
- if (upperCaseFunctionName.startsWith("PERCENTILE")) {
- String remainingFunctionName = upperCaseFunctionName.substring(10);
+ if (aggregationFunctionType.getName().toUpperCase().startsWith("PERCENTILE")) {
Review comment:
We defined enums as:
```
PERCENTILEMV("percentileMV"),
PERCENTILEESTMV("percentileEstMV"),
PERCENTILETDIGESTMV("percentileTDigestMV"),
```
----------------------------------------------------------------
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 a change in pull request #5795: Support aggregation function name with underscore inside
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-pinot/pull/5795#discussion_r464699619
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/AggregationFunctionType.java
##########
@@ -79,7 +79,7 @@ public boolean isOfType(AggregationFunctionType... aggregationFunctionTypes) {
* Returns the corresponding aggregation function type for the given function name.
*/
public static AggregationFunctionType getAggregationFunctionType(String functionName) {
- String upperCaseFunctionName = functionName.toUpperCase();
+ String upperCaseFunctionName = functionName.toUpperCase().replace("_", "");
Review comment:
will do that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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