LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-2068825265
Thanks for the reviews @gianm @kgyrtkirk @clintropolis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-2068824109
The coverage checks are failing, but due to the refactoring done on
unrelated classes - DumpSegment, MaterializedVeiwQueryToolchest. There's also
one [Jacoco
LakshSingla merged PR #15420:
URL: https://github.com/apache/druid/pull/15420
--
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:
gianm commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1568243893
##
processing/src/main/java/org/apache/druid/query/QueryResourceId.java:
##
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-2047121929
@gianm
Thanks for taking another look. I have addressed the review comments.
Regarding the relevance and the use case of the `willMergeRunner`, I have
reworded the docs. LMK in
LakshSingla commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1547144457
##
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java:
##
@@ -102,35 +104,53 @@ public class GroupByQueryQueryToolChest
gianm commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1546905734
##
processing/src/main/java/org/apache/druid/query/FluentQueryRunner.java:
##
@@ -90,9 +90,9 @@ public FluentQueryRunner
postProcess(PostProcessingOperator
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1970680139
@gianm The PR is ready for review now. There are a few code coverage
failures, but I think they are due to making changes in the unrelated classes
using the
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1970631403
@gianm With the recent changes made to the PR, I wanted to revisit the
questions posed
[here](https://github.com/apache/druid/pull/15420#issuecomment-1878164205), and
more, that
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1966918130
@gianm I am working on fixing up the ITs and the UTs due to the changed
semantics of resource passing.
A few issues I ran into were sharing of the same resource ID on the
gianm commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1962747819
@LakshSingla is this ready for another review or are some other changes
pending?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to
github-advanced-security[bot] commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1500021853
##
processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java:
##
@@ -380,16 +387,19 @@
// one
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1898301692
@gianm
> The new context keys add to complexity because there's no mechanism that
ensures they are set at the appropriate places, so it's tough to understand how
and when they're
gianm commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1886424908
I'm hoping we can fix the bug without making the code too much more complex.
There's some things that contribute to the complexity and I'm hoping we can do
some simpler alternative:
clintropolis commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1448191941
##
processing/src/main/java/org/apache/druid/query/groupby/GroupByUtils.java:
##
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1881038647
Regarding acquisition and freeing up the merge buffers, the
`GroupByResources` instance will acquire the merge buffers when created with
the appropriate number. It internally
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1880425801
To disambiguate between these cases, there's a flag called
`CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2` which determines
if the query will use the mergeRunner
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1880421795
Adding a comment here as well to aid in the code walkthrough, and for anyone
revisiting the PR:
The number of merge buffers required to execute a group by query is
calculated
LakshSingla commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1878983991
Yes @abhishekagarwal87, if the flag is set, then the outer query along with
the subquery is sent to the historicals. Else, only the subquery is sent to the
historical and the broker
gianm commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1878919490
> Curious when do we push down a subquery to historical? Where does that
logic sit?
I think @LakshSingla is talking about `forcePushDownNestedQuery`, added in
#5471, an undocumented
abhishekagarwal87 commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1878359196
Curious when do we push down a subquery to historical? Where does that logic
sit?
--
This is an automated message from the Apache Git Service.
To respond to the message,
gianm commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1878165342
One thing I'm wondering is what we need
`GroupByUtils.CTX_KEY_RUNNER_MERGES_USING_GROUP_BY_MERGING_QUERY_RUNNER_V2`
for. It shows up in places that should not need to be aware of the
gianm commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1878164205
@LakshSingla TY for the patch! Would you mind leaving an explanation in a
comment here about how the fix works? Like:
- what code is responsible for figuring out how many merge
github-advanced-security[bot] commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1409028426
##
processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java:
##
@@ -469,19 +496,37 @@
// one
LakshSingla commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1408780032
##
processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java:
##
@@ -127,17 +123,32 @@ public GroupingEngine(
*
* @return broker resource
kgyrtkirk commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1407916915
##
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunnerV2.java:
##
@@ -311,39 +310,26 @@ public void
kgyrtkirk commented on PR #15420:
URL: https://github.com/apache/druid/pull/15420#issuecomment-1830030636
I've experimented a bit with the above stuff - and although it could work -
its not that much different; it needed a different refactor to be done...
--
This is an automated message
kgyrtkirk commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1407604341
##
processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java:
##
@@ -127,17 +123,32 @@ public GroupingEngine(
*
* @return broker resource
github-advanced-security[bot] commented on code in PR #15420:
URL: https://github.com/apache/druid/pull/15420#discussion_r1406769405
##
processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownMultiNodeMergeTest.java:
##
@@ -627,23 +626,26 @@
@Test
29 matches
Mail list logo
