You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "somandal (via GitHub)" <gi...@apache.org> on 2023/04/19 20:10:40 UTC
[GitHub] [pinot] somandal opened a new pull request, #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
somandal opened a new pull request, #10649:
URL: https://github.com/apache/pinot/pull/10649
Today the Percentile TDigest aggregation functions use a hard-coded default compression factor of 100. This compressoin factor may not always be ideal especially in scenarios where more accuracy is desired. This PR adds support to optionally specify a custom compression factor as part of the query (takes a 3rd query argument). Defaults to the current default of 100 if the compression factor isn't present.
Examples of queries with the compression factor set (and not set):
- `SELECT PERCENTILETDIGESTMV(colmv, 99.99, 1000), PERCENTILETDIGEST(colsv, 98, 500) FROM table` -> uses compression factor 1000 for the first MV column and 500 for the second SV column
- `SELECT PERCENTILETDIGESTMV(colmv, 99.99), PERCENTILETDIGEST(colsv, 98) FROM table` -> defaults to compression factor of 100
- `SELECT PERCENTILETDIGEST99MV(colmv), PERCENTILETDIGEST50(colsv) FROM table` -> defaults to compression factor of 100
cc @jasperjiaguo @siddharthteotia @Jackie-Jiang
--
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] somandal commented on pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #10649:
URL: https://github.com/apache/pinot/pull/10649#issuecomment-1522238782
> @somandal This is great! Can you help update the pinot doc about this new argument?
@Jackie-Jiang thanks, I've already updated the OSS docs:
https://docs.pinot.apache.org/users/user-guide-query/supported-aggregations
https://docs.pinot.apache.org/configuration-reference/functions/percentiletdigest
https://docs.pinot.apache.org/configuration-reference/functions/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.
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171869063
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
Oh i see, do you recommend not adding the compressionFactor at all to this columnName 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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171879328
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
(nit)`<2-digits>.<16-digits> ` not sure what this exactly represents but I think compression should be specified as positive integer.
--
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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171874154
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactoryTest.java:
##########
@@ -255,25 +255,37 @@ public void testGetAggregationFunction() {
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0, 100.0)");
function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 100.0)");
+
+ function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999, 1000)");
+ aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
+ assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
+ assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 1000.0)");
function = getFunction("PeRcEnTiLeRaWtDiGeSt", "(column, 99)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileRawTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILERAWTDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0, 100.0)");
Review Comment:
cc @somandal @jasperjiaguo
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
+1 I just gave a similar comment too. Result column name should not change imo.
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171860071
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -38,21 +38,32 @@
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
Review Comment:
We should probably update update the user doc with some very simple instructions eg "typically compression=100 is used. But the user can use higher compression for more accurate results, with higher memory consumption."
--
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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171881850
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
Yes I don't think result column name should be changed and we should try to avoid changing existing expected output and instead add net new tests / expected rows
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171879328
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
`<2-digits>.<16-digits> ` not sure what this exactly represents but I think compression should be specified as positive integer.
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171874157
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
We can print only it's not 100 (default)
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171868612
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
I've modified the `getResultColumnName()` to always print the compression factor for v1 type queries. Do you recommend not printing that unless a non-default is used?
I can add additional result rows for the new syntax
--
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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171866909
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
If the 3rd argument (comp factor) is optional and default behavior is same then the existing test results don't need to change and we can add new expected result rows ?
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171902868
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -38,21 +38,32 @@
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
Review Comment:
will do once this change makes it in :)
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171854903
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
Would this change break some existing readers depending on this column name?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -38,21 +38,32 @@
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
Review Comment:
Can we update the user doc with some very simple instructions eg "typically compression=100 is used. But the user can use higher compression for more accurate results, with higher memory consumption."
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171854903
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
Would this change break some existing readers depending on this column name? Same for other change to getColumnName
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171930436
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
```
2023/04/19 22:48:29.807 ERROR [SqlResultComparator] [main] Got exception: [{"errorCode":200,"message":"QueryExecutionError:\norg.apache.pinot.spi.exception.BadQueryRequestException: Invalid aggregation function: percentiletdigest(longDimSV1,'80','1000')\n\tat org.apache.pinot.core.query.aggregation.function.AggregationFunctionFactory.getAggregationFunction(AggregationFunctionFactory.java:240)\n\tat org.apache.pinot.core.query.request.context.QueryContext$Builder.generateAggregationFunctions(QueryContext.java:457)\n\tat org.apache.pinot.core.query.request.context.QueryContext$Builder.build(QueryContext.java:433)\n\tat org.apache.pinot.core.query.request.context.utils.BrokerRequestToQueryContextConverter.convertSQL(BrokerRequestToQueryContextConverter.java:150)"}] when querying!
```
@jasperjiaguo suggested removing the new query pattern from the compatibility testing file for now since the older pattern isn't supported on older versions. Will add these tests later on
--
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] jackjlli commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171879589
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
+1 on adding additional result rows for the new syntax
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171854903
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunction.java:
##########
@@ -59,11 +64,12 @@ public AggregationFunctionType getType() {
@Override
public String getResultColumnName() {
final double percentile = _percentileTDigestAggregationFunction._percentile;
+ final double compressionFactor = _percentileTDigestAggregationFunction._compressionFactor;
final int version = _percentileTDigestAggregationFunction._version;
final String type = getType().getName().toLowerCase();
return version == 0 ? type + (int) percentile + "(" + _expression + ")"
- : type + "(" + _expression + ", " + percentile + ")";
+ : type + "(" + _expression + ", " + percentile + ", " + compressionFactor + ")";
Review Comment:
Would this change break some existing readers depending on this column name? Same for other changes to getColumnName.
--
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] siddharthteotia commented on pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #10649:
URL: https://github.com/apache/pinot/pull/10649#issuecomment-1515524026
The test failures don't seem to be related to this PR. Other PRs seem to be seeing the same env issues.
Locally tests and build are passing so merging.
--
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 #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10649:
URL: https://github.com/apache/pinot/pull/10649#issuecomment-1522235779
@somandal This is great! Can you help update the pinot doc about this new argument?
--
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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171882523
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
We are expecting compression factor to also have both precision and scale type decimal spec ?
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171902582
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactoryTest.java:
##########
@@ -255,25 +255,37 @@ public void testGetAggregationFunction() {
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0, 100.0)");
function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 100.0)");
+
+ function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999, 1000)");
+ aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
+ assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
+ assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 1000.0)");
function = getFunction("PeRcEnTiLeRaWtDiGeSt", "(column, 99)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileRawTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILERAWTDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0, 100.0)");
Review Comment:
done, thanks for the suggestions @jasperjiaguo and @siddharthteotia
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -143,6 +143,31 @@ public static AggregationFunction getAggregationFunction(FunctionContext functio
// PercentileRawTDigestMV
return new PercentileRawTDigestMVAggregationFunction(firstArgument, percentile);
}
+ } else if (numArguments == 3) {
+ // Triple arguments percentile (e.g. percentileTDigest(bar, 95, 1000), etc.) where the
+ // second argument is a decimal number from 0.0 to 100.0 and third argument is a decimal number indicating
+ // the compression_factor for the TDigest. This can only be used for TDigest type percentile functions to
+ // pass in a custom compression_factor. If the two argument version is used the default compression_factor
+ // of 100.0 is used.
+ // Have to use literal string because we need to cast int to double here.
+ double percentile = parsePercentileToDouble(arguments.get(1).getLiteral().getStringValue());
+ double compressionFactor = parseCompressionFactorToDouble(arguments.get(2).getLiteral().getStringValue());
Review Comment:
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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171869750
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -143,6 +143,31 @@ public static AggregationFunction getAggregationFunction(FunctionContext functio
// PercentileRawTDigestMV
return new PercentileRawTDigestMVAggregationFunction(firstArgument, percentile);
}
+ } else if (numArguments == 3) {
+ // Triple arguments percentile (e.g. percentileTDigest(bar, 95, 1000), etc.) where the
+ // second argument is a decimal number from 0.0 to 100.0 and third argument is a decimal number indicating
+ // the compression_factor for the TDigest. This can only be used for TDigest type percentile functions to
+ // pass in a custom compression_factor. If the two argument version is used the default compression_factor
+ // of 100.0 is used.
+ // Have to use literal string because we need to cast int to double here.
+ double percentile = parsePercentileToDouble(arguments.get(1).getLiteral().getStringValue());
+ double compressionFactor = parseCompressionFactorToDouble(arguments.get(2).getLiteral().getStringValue());
Review Comment:
Should we validate compressionFactor > 0 or greater than some minimum ?
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171903597
##########
compatibility-verifier/sample-test-suite/config/query-results/feature-test-1-rest-sql.results:
##########
@@ -21,7 +21,7 @@
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG"],"columnNames":["count(*)"]},"rows":[[10]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":4,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["sum(intMetric1)","summv(intDimMV1)","min(intMetric1)","minmv(intDimMV2)","max(longDimSV1)","maxmv(intDimMV1)"]},"rows":[[4.294967536E9,-2.147479976E9,0.0,6.0,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":5,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
{"resultTable":{"dataSchema":{"columnDataTypes":["LONG","LONG","DOUBLE","DOUBLE","DOUBLE","DOUBLE"],"columnNames":["count(*)","countmv(intDimMV1)","avg(floatMetric1)","avgmv(intDimMV2)","minmaxrange(doubleMetric1)","minmaxrangemv(intDimMV2)"]},"rows":[[10,19,114.09000263214111,1516.9,250.00000000000003,6656.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":40,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":7,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
-{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0)","percentiletdigestmv(intDimMV1, 90.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
+{"resultTable":{"dataSchema":{"columnDataTypes":["DOUBLE","DOUBLE","LONG","LONG","DOUBLE","DOUBLE"],"columnNames":["percentile(longDimSV1, 80.0)","percentilemv(intDimMV1, 90.0)","percentileest(longDimSV1, 80.0)","percentileestmv(intDimMV1, 90.0)","percentiletdigest(longDimSV1, 80.0, 100.0)","percentiletdigestmv(intDimMV1, 90.0, 100.0)"]},"rows":[[7611.0,462.0,7611,462,7611.0,462.0]]},"exceptions":[],"numServersQueried":1,"numServersResponded":1,"numSegmentsQueried":1,"numSegmentsProcessed":1,"numSegmentsMatched":1,"numDocsScanned":10,"numEntriesScannedPostFilter":20,"numGroupsLimitReached":false,"totalDocs":10,"timeUsedMs":8,"segmentStatistics":[],"traceInfo":{},"minConsumingFreshnessTimeMs":0}
Review Comment:
i added a couple of queries with the new syntax on one of the files - waiting for compatibility checker to check if the values are the same or not (since the accuracy of compression factor may change the values)
--
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] somandal commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171902369
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
the TDigest API takes the compressionFactor as a double, but I checked the code and found it uses Math.ceil() to round up :) changed this to int
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
the TDigest API takes the compressionFactor as a double, but I checked the code and found it uses Math.ceil() to round up :) changed this to int
--
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] jasperjiaguo commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171879328
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -34,25 +34,40 @@
/**
* TDigest based Percentile aggregation function.
+ *
+ * TODO: Decided not to support custom compression for version 0 queries as it seems to be the older syntax and requires
+ * extra handling for two argument PERCENTILE functions to assess if v0 or v1. This can be revisited later if the
+ * need arises
*/
public class PercentileTDigestAggregationFunction extends BaseSingleInputAggregationFunction<TDigest, Double> {
public static final int DEFAULT_TDIGEST_COMPRESSION = 100;
- //version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column)
- //version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>)
+ // version 0 functions specified in the of form PERCENTILETDIGEST<2-digits>(column). Uses default compression of 100
+ // version 1 functions of form PERCENTILETDIGEST(column, <2-digits>.<16-digits>, <2-digits>.<16-digits> [optional])
Review Comment:
`<2-digits>.<16-digits> ` not sure what this exactly represents but I think compression should be specified as integer.
--
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] siddharthteotia merged pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia merged PR #10649:
URL: https://github.com/apache/pinot/pull/10649
--
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 #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10649:
URL: https://github.com/apache/pinot/pull/10649#issuecomment-1515375738
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10649?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 [#10649](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8317a56) into [master](https://codecov.io/gh/apache/pinot/commit/f2afe21b9962a0d676a7887edc999bcbee6c441d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f2afe21) will **decrease** coverage by `38.19%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10649 +/- ##
=============================================
- Coverage 70.31% 32.13% -38.19%
+ Complexity 6495 453 -6042
=============================================
Files 2106 2106
Lines 113381 113470 +89
Branches 17090 17102 +12
=============================================
- Hits 79728 36463 -43265
- Misses 28059 73722 +45663
+ Partials 5594 3285 -2309
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.40% <0.00%> (+0.02%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.89% <0.00%> (+0.02%)` | :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/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...gregation/function/AggregationFunctionFactory.java](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uRmFjdG9yeS5qYXZh) | `9.93% <0.00%> (-72.62%)` | :arrow_down: |
| [...ction/PercentileRawTDigestAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9QZXJjZW50aWxlUmF3VERpZ2VzdEFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ion/PercentileRawTDigestMVAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9QZXJjZW50aWxlUmF3VERpZ2VzdE1WQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-60.00%)` | :arrow_down: |
| [...function/PercentileTDigestAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9QZXJjZW50aWxlVERpZ2VzdEFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-80.22%)` | :arrow_down: |
| [...nction/PercentileTDigestMVAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/10649?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9QZXJjZW50aWxlVERpZ2VzdE1WQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
... and [1318 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10649/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] siddharthteotia commented on a diff in pull request #10649: Add support for custom compression factor for Percentile TDigest aggregation functions
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10649:
URL: https://github.com/apache/pinot/pull/10649#discussion_r1171873534
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactoryTest.java:
##########
@@ -255,25 +255,37 @@ public void testGetAggregationFunction() {
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.0, 100.0)");
function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 100.0)");
+
+ function = getFunction("PeRcEnTiLeTdIgEsT", "(column, 99.9999, 1000)");
+ aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
+ assertTrue(aggregationFunction instanceof PercentileTDigestAggregationFunction);
+ assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILETDIGEST);
+ assertEquals(aggregationFunction.getResultColumnName(), "percentiletdigest(column, 99.9999, 1000.0)");
function = getFunction("PeRcEnTiLeRaWtDiGeSt", "(column, 99)");
aggregationFunction = AggregationFunctionFactory.getAggregationFunction(function, DUMMY_QUERY_CONTEXT);
assertTrue(aggregationFunction instanceof PercentileRawTDigestAggregationFunction);
assertEquals(aggregationFunction.getType(), AggregationFunctionType.PERCENTILERAWTDIGEST);
- assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0)");
+ assertEquals(aggregationFunction.getResultColumnName(), "percentilerawtdigest(column, 99.0, 100.0)");
Review Comment:
Why should the result column name change ?
If they never specified the comp factor in the argument, then they know it will default to whatever default compression factor and then no need to expose it to them via result column name.
If they specified the 3rd argument, then just like every function we include it as is in the result column name imo
--
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