You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2024/07/16 13:31:36 UTC
[PR] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
yashmayya opened a new pull request, #13628:
URL: https://github.com/apache/pinot/pull/13628
- With the v2 / multi-stage query engine becoming more robust and feature complete (https://github.com/apache/pinot/pulls?q=is%3Apr+label%3Amulti-stage+is%3Aclosed), existing users might want to potentially migrate their existing v1 query workload to the multi-stage query engine.
- However, there are some differences between the two query engines in terms of supported syntax (some of which is documented in https://docs.pinot.apache.org/reference/troubleshooting/troubleshoot-multi-stage-query-engine).
- In case a user wants to migrate from the v1 query engine to the v2 query engine, it would be useful to be able to automatically detect whether the existing query workload would work as is without modification. This patch adds a counter metric (optionally enabled via a broker configuration that defaults to false for performance reasons) that is incremented each time a v1 query is run that fails during compilation with the v2 query engine. If this counter remains 0 during regular query workload execution, it signals that users can potentially migrate their query workload to the multi-stage query engine.
- Note that there could be situations where a query compiles successfully but fails during execution. There could also be cases where a query runs successfully but there are differences in the result between the v1 and the v2 query engine - in terms of result types, result set size etc.
- In order to address these above concerns, we plan to also add a tool / API in the near future that actually executes a given query against both the query engines and compares the results. This is much more heavyweight and shouldn't be done for every query during regular execution and is intended more for ad-hoc use cases.
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1685974950
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
Alternatively, we could potentially use a lock-free bounded queue (like [BoundedFifoBuffer](https://commons.apache.org/proper/commons-collections/javadocs/api-3.2.2/org/apache/commons/collections/buffer/BoundedFifoBuffer.html) from `commons-collections`) with some additional logic on our end to avoid busy waiting on the consumer side when the buffer is empty. Or, we could use Java's [ConcurrentLinkedQueue](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html) which is lock-free but unbounded and add some logic on our end to make it bounded (and also the logic for avoiding busy waiting on the consumer side when the queue is empty). Neither option is too appealing though 😄
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1683870200
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
Can we also add a config to sample the queries we are going to compare. I mean, instead of compiling every query in V2, we can say something like: compile 1 every X queries or a X% of the queries.
Given the high QPS Pinot usually has, this sampling should be good enough to provide a good enough overview without having to parse all queries in V2, which may add some impact.
Alternatively we can delegate this into a background thread that may be reading from a concurrent queue, so running V1 queries won't be affected by the time spent in planning V2. The important part here is that the thread adding into the queue should be using [Queue.offer](https://docs.oracle.com/javase/8/docs/api/java/util/Queue.html#offer-E-) so if the queue is full it just continues (the query won't be reported, but at the same time it won't be blocked).
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1685965011
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
I've gone with a simple single threaded executor and a blocking queue implementation for now, but on second thoughts I think we might run into contention issues in high QPS environments with the request processing threads all vying for the blocking queue's internal lock during the call to `offer`. I think we could potentially solve this using our own additional external lock and just drop requests if the lock isn't free (using [Lock.tryLock](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/Lock.html#tryLock--)). Although this would still introduce some additional overhead. Thoughts?
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1694984337
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
I don't think that lock will be that problematic. Given this feature is behind a config flag, it can be enabled as needed. In case ends up being a performance issue we can just turn it off.
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1685974950
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
Alternatively, we could potentially use a lock-free bounded queue (like [BoundedFifoBuffer](https://commons.apache.org/proper/commons-collections/javadocs/api-3.2.2/org/apache/commons/collections/buffer/BoundedFifoBuffer.html) from `commons-collections`) with some additional logic on our end to avoid busy waiting on the consumer side when the buffer is empty. Or, we could use Java's [ConcurrentLinkedQueue](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html) which is lock-free but unbounded and add some logic on our end to make it bounded (and also the logic for avoiding busy waiting on the consumer side when the queue is empty). Neither option is too appealing though 😄
Edit: for the waiting logic we'd need our own external lock in any case, so we could just wrap a regular non-synchronized lock-free bounded queue.
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #13628:
URL: https://github.com/apache/pinot/pull/13628#issuecomment-2230936245
Metric name and broker config are up for debate (naming is hard) 😄
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13628:
URL: https://github.com/apache/pinot/pull/13628#issuecomment-2231068266
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13628?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `26.08696%` with `17 lines` in your changes missing coverage. Please review.
> Project coverage is 46.34%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6f1bc80`)](https://app.codecov.io/gh/apache/pinot/commit/6f1bc805ce317497d15c09c73f66a67dfaab435b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 764 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13628?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../java/org/apache/pinot/query/QueryEnvironment.java](https://app.codecov.io/gh/apache/pinot/pull/13628?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2FQueryEnvironment.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvUXVlcnlFbnZpcm9ubWVudC5qYXZh) | 26.66% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13628?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/pinot/query/parser/utils/ParserUtils.java](https://app.codecov.io/gh/apache/pinot/pull/13628?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fparser%2Futils%2FParserUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGFyc2VyL3V0aWxzL1BhcnNlclV0aWxzLmphdmE=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13628?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 (59551e4) and HEAD (6f1bc80). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (6f1bc80) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13628 +/- ##
=============================================
- Coverage 61.75% 46.34% -15.41%
- Complexity 207 1113 +906
=============================================
Files 2436 1949 -487
Lines 133233 103737 -29496
Branches 20636 16788 -3848
=============================================
- Hits 82274 48076 -34198
- Misses 44911 52011 +7100
+ Partials 6048 3650 -2398
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13628/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/13628/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/13628/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/13628/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/13628/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/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.34% <26.08%> (-15.29%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.32% <26.08%> (-15.43%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.30% <26.08%> (+18.57%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.34% <26.08%> (-15.41%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.34% <26.08%> (-15.41%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.34% <26.08%> (-0.55%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13628/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/13628?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).
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #13628:
URL: https://github.com/apache/pinot/pull/13628#issuecomment-2255572371
Approved. Waiting to fix the conflicts, which should be pretty simple given they are in the CommonConstants.
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1680430202
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -190,14 +194,32 @@ public List<String> getTableNamesForQuery(String sqlQuery) {
if (sqlNode.getKind().equals(SqlKind.EXPLAIN)) {
sqlNode = ((SqlExplain) sqlNode).getExplicandum();
}
- RelRoot relRoot = compileQuery(sqlNode, plannerContext);
+ RelRoot relRoot = compileAndOptimizeQuery(sqlNode, plannerContext);
Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
return new ArrayList<>(tableNames);
} catch (Throwable t) {
throw new RuntimeException("Error composing query plan for: " + sqlQuery, t);
}
}
+ /**
+ * Returns whether the query can be successfully compiled in this query environment
+ */
+ public boolean canCompileQuery(String query) {
+ try (PlannerContext plannerContext = getPlannerContext()) {
+ SqlNode sqlNode = CalciteSqlParser.compileToSqlNodeAndOptions(query).getSqlNode();
+ if (sqlNode.getKind().equals(SqlKind.EXPLAIN)) {
+ sqlNode = ((SqlExplain) sqlNode).getExplicandum();
+ }
+ compileAndOptimizeQuery(sqlNode, plannerContext);
Review Comment:
In the initial version, I'd skipped the optimize step here for perf reasons but turns out that there are some cases where bad queries are only caught in the optimize step. For instance, [this test](https://github.com/apache/pinot/blob/17332dea90183323f1eb3f1d15f01ddfdf94beee/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java#L1058) started failing because the bad query compiles fine but the `PinotEvaluateLiteralRule` catches the issue during the optimize query phase.
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #13628:
URL: https://github.com/apache/pinot/pull/13628
--
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] Automatically detect whether a v1 query could have run on the v2 query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13628:
URL: https://github.com/apache/pinot/pull/13628#discussion_r1685961517
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -478,8 +481,17 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
}
_brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.QUERIES, 1);
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERIES_GLOBAL, 1);
_brokerMetrics.addValueToTableGauge(rawTableName, BrokerGauge.REQUEST_SIZE, query.length());
+ if (!pinotQuery.isExplain() && _enableMultistageMigrationMetric) {
Review Comment:
Thanks for the suggestions! I do agree that we need to be careful about affecting query latency in the v1 engine due to this additional query compilation overhead. I liked your second option more because the first option can still introduce unpredictable latency in some queries which can increase the tail latency by a fair amount. I did some ad-hoc benchmarking and for some queries, the v2 engine query compilation time is actually comparable in order of magnitude to the overall v1 query execution time (and I'm sure in certain cases it could even be higher).
Also, I'm not a big fan of adding more user configuration than is necessary. I've gone with the second option for now - background thread processing (v2 query compiling) a queue of v1 query requests where the queue capacity is currently hardcoded / not configurable. We could make this queue capacity configurable in the future if we see a need for allowing this to be configurable by users - i.e., in order to tune the percentage of requests that are effectively sampled based on the query workload.
--
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