You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "songwdfu (via GitHub)" <gi...@apache.org> on 2025/07/09 18:38:42 UTC
[PR] Optimize QueryContext.isUnsafeTrim [pinot]
songwdfu opened a new pull request, #16312:
URL: https://github.com/apache/pinot/pull/16312
change implementation to comparing set of column identifiers. cache after first call to the function.
--
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] Optimize QueryContext.isUnsafeTrim [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16312:
URL: https://github.com/apache/pinot/pull/16312#discussion_r2196051540
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -175,36 +175,10 @@ private boolean isSameOrderAndGroupByColumns(QueryContext context) {
return false;
}
- BitSet orderByKeysMatched = new BitSet(orderByKeys.size());
-
- OUTER_GROUP:
- for (ExpressionContext groupExp : groupByKeys) {
- for (int i = 0; i < orderByKeys.size(); i++) {
- OrderByExpressionContext orderExp = orderByKeys.get(i);
- if (groupExp.equals(orderExp.getExpression())) {
- orderByKeysMatched.set(i);
- continue OUTER_GROUP;
- }
- }
-
- return false;
- }
-
- OUTER_ORDER:
- for (int i = 0, n = orderByKeys.size(); i < n; i++) {
- if (orderByKeysMatched.get(i)) {
- continue;
- }
-
- for (ExpressionContext groupExp : groupByKeys) {
- if (groupExp.equals(orderByKeys.get(i).getExpression())) {
- continue OUTER_ORDER;
- }
- }
- return false;
- }
-
- return true;
+ Set<ExpressionContext> groupByKeyIdentSet = new HashSet<>(groupByKeys);
Review Comment:
@bziobrowski Is there a specific reason why you are doing 2 loops instead of comparing the set?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -564,7 +538,11 @@ public boolean isIndexUseAllowed(DataSource dataSource, FieldConfig.IndexType in
}
public boolean isUnsafeTrim() {
- return !isSameOrderAndGroupByColumns(this) || getHavingFilter() != null;
+ if (_isUnsafeTrim != null) {
Review Comment:
We usually pre-compute this during the `build()` call. We can do the same.
Currently this can still be computed multiple times
--
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] Optimize QueryContext.isUnsafeTrim [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16312:
URL: https://github.com/apache/pinot/pull/16312#issuecomment-3053743655
### :x: Unsupported file format
<details><summary>Upload processing failed due to unsupported file format. Please review the parser error message:</summary>
<pre><code class="language-python">Error parsing JUnit XML in /home/runner/work/pinot/pinot/pinot-query-runtime/target/surefire-reports/TEST-org.apache.pinot.query.runtime.queries.ResourceBasedQueriesTest.xml at 136:1061
Caused by:
RuntimeError: Error converting computed name to ValidatedString
Caused by:
string is too long</code></pre>
</details>
For more help, visit our [troubleshooting guide](https://docs.codecov.com/docs/test-analytics#troubleshooting).
--
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] Optimize QueryContext.isUnsafeTrim [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #16312:
URL: https://github.com/apache/pinot/pull/16312
--
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] Optimize QueryContext.isUnsafeTrim [pinot]
Posted by "bziobrowski (via GitHub)" <gi...@apache.org>.
bziobrowski commented on code in PR #16312:
URL: https://github.com/apache/pinot/pull/16312#discussion_r2200007371
##########
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java:
##########
@@ -175,36 +175,10 @@ private boolean isSameOrderAndGroupByColumns(QueryContext context) {
return false;
}
- BitSet orderByKeysMatched = new BitSet(orderByKeys.size());
-
- OUTER_GROUP:
- for (ExpressionContext groupExp : groupByKeys) {
- for (int i = 0; i < orderByKeys.size(); i++) {
- OrderByExpressionContext orderExp = orderByKeys.get(i);
- if (groupExp.equals(orderExp.getExpression())) {
- orderByKeysMatched.set(i);
- continue OUTER_GROUP;
- }
- }
-
- return false;
- }
-
- OUTER_ORDER:
- for (int i = 0, n = orderByKeys.size(); i < n; i++) {
- if (orderByKeysMatched.get(i)) {
- continue;
- }
-
- for (ExpressionContext groupExp : groupByKeys) {
- if (groupExp.equals(orderByKeys.get(i).getExpression())) {
- continue OUTER_ORDER;
- }
- }
- return false;
- }
-
- return true;
+ Set<ExpressionContext> groupByKeyIdentSet = new HashSet<>(groupByKeys);
Review Comment:
That was my habit of trying to make code as allocation-free as possible .
--
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