You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2023/08/18 01:20:09 UTC
[PR] remove group-by v1 (druid)
clintropolis opened a new pull request, #14866:
URL: https://github.com/apache/druid/pull/14866
### Description
This PR removes the 'v1' grouping engine as well as the strategy selection abstraction. I found `GroupByStrategyV2` to still be a relatively handy thing to keep around since it is a nice container for grabbing the resources and stuff needed and plumbing to the underlying group by v2 engine, so I have transitioned it into a new class `GroupingEngine` which is now used in place of prior uses of `GroupByStrategySelector`.
#### Release note
The V1 group by query has been removed in favor of always using the V2 engine. Any query context parameters associated with it will now be ignored internally.
`AggregatorFactory.getRequiredColumns` has been deprecated and will be removed in a future release. If you have an extension that implements `AggregatorFactory` then this method should be removed from your implementation (there is now a default definition that throws an exception about being deprecated).
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302441567
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
i mean, its in the middle of a sentence, why would we uppercase it
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302460313
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
I guess like... I disagree, but we have a lot of other links doing that too.
Like take this example:
<img width="904" alt="Screenshot 2023-08-22 at 9 08 33 PM" src="https://github.com/apache/druid/assets/1577461/1e43e967-3ccc-4848-b4e1-e100ba59341f">
Why should the A in "Advance groupBy v2 configurations" and the M in "Memory tuning and resource limits" be uppercase?
Is this a matter of perspective? Like it bothers me because I view these inline links as just some text that is part of a sentence that happen to be clickable. But i guess another perspective might be that this is some proper section title and it should match as closely as possible? I still don't really personally like it, but i changed it because a handful of other links are like this on this and other pages (not all of them 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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302460313
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
I guess like... I disagree, but we have a lot of other links doing that too.
Like take this example:
<img width="904" alt="Screenshot 2023-08-22 at 9 08 33 PM" src="https://github.com/apache/druid/assets/1577461/1e43e967-3ccc-4848-b4e1-e100ba59341f">
Why should the A in "Advance groupBy v2 configurations" and the M in "Memory tuning and resource limits" be uppercase?
Is this a matter of perspective? Like it bothers me because these inline links are just some text that is part of a sentence that happen to be clickable. But i guess another perspective might be that this is some proper section title and it should match as closely as possible? I still don't really personally like it, but i changed it because a handful of other links are like this on this and other pages (not all of them 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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "ektravel (via GitHub)" <gi...@apache.org>.
ektravel commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1300360665
##########
docs/querying/groupbyquery.md:
##########
@@ -440,6 +372,7 @@ Supported query contexts:
|Key|Description|Default|
|---|-----------|-------|
+|`groupByIsSingleThreaded`|Overrides the value of `druid.query.groupBy.singleThreaded` for this query.|
Review Comment:
```suggestion
|`groupByIsSingleThreaded`|Overrides the value of `druid.query.groupBy.singleThreaded` for this query.|None|
```
The entry on line 375 is missing the 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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1303301058
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
IMO @ektravel's perspective makes sense. If the link text is a title of a page and is referring to the page itself then capitalizing the first letter seems good. It emphasizes that we're referring to a documentation page rather than a concept.
To me this is correct (the text refers to the page being linked to):
```
See [Query caching](../querying/caching.md) for more information.
```
And this is also correct (the text refers to a concept, not the page being linked to):
```
It is important to set up [query caching](../querying/caching.md) properly for best performance.
```
Btw, the following is also IMO correct, but awkward and I'd avoid it in favor of the first example. Here "query caching" is a common noun acting as an adjective for "documentation"; it's not referring to the page being linked to, so it shouldn't be capitalized:
```
See the [query caching](../querying/caching.md) documentation for more information.
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #14866:
URL: https://github.com/apache/druid/pull/14866#issuecomment-1686558352
> I actually did this initially, but then noticed that #14462 was also making this change, so reverted for now. That said, it would only be a minor conflict with the other PR, so Im willing to add it back.
OK, fair enough. We can leave it for #14462.
> Yeah, totally, i actually imagine a lot of other parts of IncrementalIndex could also be improved. Should I do that in this PR or save it for a follow-up? In the very least i can remove the group by v1 comment in addToFacts and add another comment to replace it indicating that the condition should now be impossible and we should work to simplify the code area in the future
Up to you how much you want to do in this PR. I'd at least adjust the comment in there that mentions groupBy v1. Other stuff could be done as future work.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1303301058
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
IMO @ektravel's perspective makes sense. If the link text is a title of a page and is referring to the page itself then capitalizing the first letter seems good. It emphasizes that we're referring to a documentation page rather than a concept.
To me this is correct (the text refers to the page being linked to):
```
See [Query caching](../querying/caching.md) for more information.
```
And this is also correct (the text refers to a concept, not the page being linked to):
```
It is important to set up [query caching](../querying/caching.md) properly for best performance.
```
Btw, the following is also IMO correct, but awkward and I'd avoid it in favor of the first example. Here "query caching" is a common noun acting as an adjective; it's not referring to the page being linked to, so it shouldn't be capitalized:
```
See the [query caching](../querying/caching.md) documentation for more information.
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302441263
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
I see lots of other links like this.... but why?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "ektravel (via GitHub)" <gi...@apache.org>.
ektravel commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1300354739
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
```suggestion
See [Query caching](../querying/caching.md) for more information.
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "ektravel (via GitHub)" <gi...@apache.org>.
ektravel commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1303164944
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
@clintropolis In [this specific example](https://github.com/apache/druid/pull/14866#pullrequestreview-1587410573), [Query caching](https://druid.apache.org/docs/latest/querying/caching) is the title of the page and must be referenced using sentence case.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #14866:
URL: https://github.com/apache/druid/pull/14866#issuecomment-1684488529
>longLast, longFirst, doubleFirst, doubleLast could stop lying about their intermediate types. IIRC they only had to lie about this to make groupBy v1 work.
I actually did this initially, but then noticed that #14462 was also making this change, so reverted for now. That said, it would only be a minor conflict with the other PR, so Im willing to add it back.
>OnheapIncrementalIndex no longer needs to support multithreaded writers, so addToFacts could be simplified
Yeah, totally, i actually imagine a lot of other parts of `IncrementalIndex` could also be improved. Should I do that in this PR or save it for a follow-up? In the very least i can remove the group by v1 comment in `addToFacts` and add another comment to replace it indicating that the condition should now be impossible and we should work to simplify the code area in the future
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302456298
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
FWIW, personally I find the suggested style to be better :)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #14866:
URL: https://github.com/apache/druid/pull/14866#discussion_r1302482334
##########
docs/configuration/index.md:
##########
@@ -2026,8 +2026,8 @@ You can optionally only configure caching to be enabled on the Broker by setting
See [cache configuration](#cache-configuration) for how to configure cache settings.
:::info
- Note: Even if cache is enabled, for [groupBy v2](../querying/groupbyquery.md#strategies) queries, segment level cache do not work on Brokers.
- See [Differences between v1 and v2](../querying/groupbyquery.md#differences-between-v1-and-v2) and [Query caching](../querying/caching.md) for more information.
+ Note: Even if cache is enabled, for [groupBy](../querying/groupbyquery.md) queries, segment level cache does not work on Brokers.
+ See [query caching](../querying/caching.md) for more information.
Review Comment:
For me, it seems better because upper-casing highlights that part of the text. But yes, in the end, its a matter of perspective. I don't have an objective argument for either or against.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] remove group-by v1 (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #14866:
URL: https://github.com/apache/druid/pull/14866
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org