You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/10/07 02:03:50 UTC
[PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Jackie-Jiang opened a new pull request, #16966:
URL: https://github.com/apache/pinot/pull/16966
Outer `IS_TRUE` function over filter is redundant on leaf stage, and can be trimmed off.
It can be generated from MSE when null handling is enabled
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2409668926
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
Review Comment:
I think we should call `explainAskingServers` instead. Something like:
```java
explainAskingServers(query, "Execution Plan\n"
+ "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ " PinotLogicalExchange(distribution=[hash[0]])\n"
+ " LeafStageCombineOperator(table=[mytable])\n"
+ " StreamingInstanceResponse\n"
+ " CombineGroupBy\n"
+ " GroupByFiltered(groupKeys=[[city]], aggregations=[[count(*), count(*)]])\n"
+ " Project(columns=[[city]])\n"
+ " DocIdSet(maxDocs=[20000])\n"
+ " FilterEmpty\n"
+ " Project(columns=[[city]])\n"
+ " DocIdSet(maxDocs=[20000])\n"
+ " FilterMatchEntireSegment(numDocs=[100])\n");
```
Which shows that the `IS TRUE` predicate is removed.
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411572685
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -287,6 +287,16 @@ public static String getStringValue(Expression thriftExpression) {
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(ExpressionContext filterExpression) {
+ FunctionContext function = filterExpression.getFunction();
+ // Trim off outer IS_TRUE function as it is redundant
+ if (function != null && function.getFunctionName().equals("istrue")) {
+ return getFilter(function.getArguments().get(0));
+ } else {
Review Comment:
`function != null` is equivalent to that check
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411604691
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
Review Comment:
Added
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411970162
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,45 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ if (useMultiStageQueryEngine) {
+ // MSE will insert IS_TRUE to the aggregate filter
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
+ + " PinotLogicalTableScan(table=[[default, mytable]])\n");
+ // IS_TRUE should be trimmed off, then the filter becomes always false in the server execution plan
+ explainAskingServers(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " LeafStageCombineOperator(table=[mytable])\n"
+ + " StreamingInstanceResponse\n"
+ + " CombineGroupBy\n"
+ + " GroupByFiltered(groupKeys=[[city]], aggregations=[[count(*), count(*)]])\n"
+ + " Project(columns=[[city]])\n"
+ + " DocIdSet(maxDocs=[20000])\n"
+ + " FilterEmpty\n"
Review Comment:
It looks up the dictionary when creating the physical (server) execution plan, and cannot find the value matching `'unknown'`. Without the special handling in this PR, the filter remains a UDF (`IS_TRUE(description = 'unknown')`), and we need to scan all records in order to execute the query
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411594072
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -287,6 +287,16 @@ public static String getStringValue(Expression thriftExpression) {
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(ExpressionContext filterExpression) {
+ FunctionContext function = filterExpression.getFunction();
+ // Trim off outer IS_TRUE function as it is redundant
+ if (function != null && function.getFunctionName().equals("istrue")) {
+ return getFilter(function.getArguments().get(0));
+ } else {
Review Comment:
Hm I meant the argument to the `IS_TRUE` filter function actually
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2409290390
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
Review Comment:
So this `IS_TRUE` is inserted in order to filter out null values because `=(null, 'unknown')` is `null` and `IS_TRUE(null)` evaluates to `false`?
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -287,6 +287,16 @@ public static String getStringValue(Expression thriftExpression) {
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(ExpressionContext filterExpression) {
+ FunctionContext function = filterExpression.getFunction();
+ // Trim off outer IS_TRUE function as it is redundant
+ if (function != null && function.getFunctionName().equals("istrue")) {
+ return getFilter(function.getArguments().get(0));
+ } else {
Review Comment:
Shouldn't we check if the argument is also a function? Do we want to apply this optimization even if the argument is an identifier?
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2409676759
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
Review Comment:
> And we don't want to preserve the IS_TRUE predicate because our implementation of = doesn't return null?
`IS_TRUE(x)` never returns null (in SQL). It is semantically equal to `x = true and x is not null`, which always evaluate to true or false.
I think this IS_TRUE is added by Calcite to filter out nulls on the predicate, as it should be evaluated to true or false. Am I right Jackie?
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411016446
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
Review Comment:
Yep, I'm asking about the EQUALS implementation, not IS_TRUE 😄
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411587362
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
Review Comment:
Pinot leaf stage (SSE) is able to handle this within filter clause. There are 3 possible values for predicate: `true`, `false`, `null` (unknown). SSE will keep records evaluated to `true` only, thus this `IS_TRUE` is redundant
`EQUALS` can evaluate to `null` when either the left hand side or right hand side is `null`
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2409294428
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,29 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ // MSE will insert IS_TRUE to the aggregate filter
+ if (useMultiStageQueryEngine) {
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
Review Comment:
And we don't want to preserve the `IS_TRUE` predicate because our implementation of `=` doesn't return `null`?
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16966:
URL: https://github.com/apache/pinot/pull/16966#issuecomment-3374975683
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16966?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `85.71429%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.08%. Comparing base ([`6d187f2`](https://app.codecov.io/gh/apache/pinot/commit/6d187f22f4a47beba9965a4243f575f194057abf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bf121c3`](https://app.codecov.io/gh/apache/pinot/commit/bf121c32bd032cdd4c3b548f189159b099732c1e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16966?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ot/common/request/context/RequestContextUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16966?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Frequest%2Fcontext%2FRequestContextUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L1JlcXVlc3RDb250ZXh0VXRpbHMuamF2YQ==) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16966?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (6d187f2) and HEAD (bf121c3). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (6d187f2) | HEAD (bf121c3) |
>|------|------|------|
>|java-21|5|1|
>|unittests|4|2|
>|temurin|9|2|
>|java-11|4|1|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|integration2|1|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16966 +/- ##
============================================
- Coverage 63.27% 56.08% -7.20%
+ Complexity 1411 702 -709
============================================
Files 3073 2409 -664
Lines 180529 133744 -46785
Branches 27622 21278 -6344
============================================
- Hits 114238 75009 -39229
+ Misses 57517 52518 -4999
+ Partials 8774 6217 -2557
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.04% <85.71%> (-7.19%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.03% <85.71%> (-7.22%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.08% <85.71%> (-7.20%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.08% <85.71%> (-7.20%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.08% <85.71%> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16966/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16966?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411623992
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -459,4 +459,45 @@ public Object[][] nullLiteralQueries() {
{String.format("SELECT tan(null) FROM %s", getTableName()), "null"}
};
}
+
+ /// This test ensures IS_TRUE can be trimmed off on leaf stage
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testFilteredAggregationNoScanInFilter(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ String query = "SELECT city, COUNT(*), COUNT(*) FILTER(WHERE description = 'unknown') FROM mytable GROUP BY city";
+
+ if (useMultiStageQueryEngine) {
+ // MSE will insert IS_TRUE to the aggregate filter
+ explainLogical(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " PinotLogicalAggregate(group=[{0}], agg#0=[COUNT()], agg#1=[COUNT() FILTER $1], aggType=[LEAF])\n"
+ + " LogicalProject(city=[$5], $f1=[IS TRUE(=($7, _UTF-8'unknown'))])\n"
+ + " PinotLogicalTableScan(table=[[default, mytable]])\n");
+ // IS_TRUE should be trimmed off, then the filter becomes always false in the server execution plan
+ explainAskingServers(query,
+ "Execution Plan\n"
+ + "PinotLogicalAggregate(group=[{0}], agg#0=[COUNT($1)], agg#1=[COUNT($2)], aggType=[FINAL])\n"
+ + " PinotLogicalExchange(distribution=[hash[0]])\n"
+ + " LeafStageCombineOperator(table=[mytable])\n"
+ + " StreamingInstanceResponse\n"
+ + " CombineGroupBy\n"
+ + " GroupByFiltered(groupKeys=[[city]], aggregations=[[count(*), count(*)]])\n"
+ + " Project(columns=[[city]])\n"
+ + " DocIdSet(maxDocs=[20000])\n"
+ + " FilterEmpty\n"
Review Comment:
How does the query planner know that `description = 'unknown'` is always false? 🤔
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16966:
URL: https://github.com/apache/pinot/pull/16966#discussion_r2411973678
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -287,6 +287,16 @@ public static String getStringValue(Expression thriftExpression) {
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(ExpressionContext filterExpression) {
+ FunctionContext function = filterExpression.getFunction();
+ // Trim off outer IS_TRUE function as it is redundant
+ if (function != null && function.getFunctionName().equals("istrue")) {
+ return getFilter(function.getArguments().get(0));
+ } else {
Review Comment:
We should also optimize queries such as `WHERE column IS TRUE` because it is equivalent to `WHERE column`
--
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
Re: [PR] Trim of redundant IS_TRUE function when creation filter on leaf stage [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #16966:
URL: https://github.com/apache/pinot/pull/16966
--
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