[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15407920#comment-15407920
]
Benjamin Lerer commented on CASSANDRA-10707:
The DTest results look good.
[~slebresne] Thanks for the deep reviews.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
> Fix For: 3.x
>
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15407522#comment-15407522
]
Benjamin Lerer commented on CASSANDRA-10707:
I am +1 on both commits. I ran locally the {{GROUP BY}} DTests and they all
succeeded.
I triggered a new run of the DTests and are waiting for the results.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
> Fix For: 3.x
>
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15404044#comment-15404044
]
Sylvain Lebresne commented on CASSANDRA-10707:
--
Last version mostly look good. The main thing I still don't like is the
{{filterOnReplica}} method: I feel it's easy to misuse and doesn't feel
particulary natural. Thinking about this, I feel the underlying issue we're
trying to solve is more general: the {{DataLimits}} holds state (for paging and
grouping) which somewhat assumes things are queried sequentially (and in
order). However, when we do range queries and send queries in parallel to
nodes, that's not true anymore (except maybe for the first range sent), at
least not for the queries sent to replica (we still process them in order on
the coordinator). So anyway, I think a better way to handle this is to
acknowledge that fact in {{StorageProxy.getRangeSlice}} and drop any state from
the sub-range commands sent in parallel. I've tried such change in the branch
attached below (which is also rebased).
The branch also include a commit with a few nits, mostly around comments. Feel
free to ignore some of it if you don't like it.
| [10707-trunk|https://github.com/pcmanus/cassandra/commits/10707-trunk] |
[utests|http://cassci.datastax.com/job/pcmanus-10707-trunk-testall] |
[dtests|http://cassci.datastax.com/job/pcmanus-10707-trunk-dtest] |
I'll note that the dtest run has failures, but this is a ongoing problem with
CI today. Random tests fail with {{Host has been marked down or removed}} but
you get that on today trunk run as well:
http://cassci.datastax.com/view/trunk/job/trunk_dtest/1322/
Anyway, if we can agree on those 2 small commits, then I'm +1 (though we might
want to wait on CI to stabilize on dtests to make sure).
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
> Fix For: 3.x
>
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15375590#comment-15375590
]
Benjamin Lerer commented on CASSANDRA-10707:
Thanks for the deep review.
I have merge the previous commits and pushed new ones to address the review
comments.
|[c* branch|https://github.com/blerer/cassandra/tree/10707-trunk]|[dtest
branch|https://github.com/riptano/cassandra-dtest/compare/master...blerer:CASSANDRA-10707?expand=1]|[utests|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-testall/]|[dtests|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-dtest/]|
bq. At the end of SelectStatement.getAggregationSpecification, I believe the
clusteringPrefixSize > 0 && isDistinct test is redundant since we'd have exited
early in that case.
The test is valid but it turned out that the unit tests were using
{{assertInvalid}} instead of {{assertInvalidMessage}}. I fixed that problem.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
> Fix For: 3.x
>
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15356881#comment-15356881
]
Sylvain Lebresne commented on CASSANDRA-10707:
--
Sorry for the long iterations on the reviews. I still have a bunch of remarks,
though a lot are fairly minor. In general, I "think" the general logic if fine,
but it's still a bit hard to wrap your head around so I'm also listing things
that are unclear to me, for which some extra-commenting might be just what's
missing. Anyway, here we go:
* On {{GroupMaker.State}}:
** I'd promote it to a top-level {{GroupingState}} class since it's used in
{{DataLimits}} too and would have a nice symmetry with {{PagingState}}.
** Any reason not to reuse the {{Clustering.Serializer}} in the serializer
(it probably requires keeping the CFMetaData or ClusteringComparator around for
the types but not a big deal)? In particular, the hand-rolled serialization
doesn't handle nulls (and should).
** I think it would useful to spell out in more details what having or not
having each component means. My understanding is that if {{partitionKey() !=
null}} and {{clustering == null}}, it means we haven't started counting the
partition at all (meaning that seeing that partition in the previous page made
use close a group and subsequently hit the page limit, hence stopping). But if
{{clustering != null}}, it means we stopped the previous page in the middle of
a group. But I'm not sure I'm fully correct (maybe I'm missing some edge cases
in particular?) and documenting this would be nice.
** Nit: The comment on the {{clustering()}} method is incorrect (talks about
"partition key").
* In general, the handling of the static column is a bit subtle, and it would
be useful to have a comment somewhere that explain its general handling in
details. For instance, the first case of
{{GroupByAwareCounter.applyToPartition()}} contradicts slightly my
understanding of what {{state.partitionKey()}} means. That is, I though that if
reaching a new partition {{X}} makes us close a group and hit a page limit,
then we'll stop and have {{X}} as the partitionKey in the state. However, on
the next page, it seems we assume we've somehow already accounted the static
row and I'm unclear why. The code in {{applyToStatic}} is also bemusing to me:
to start with, what is the subtle difference between the first condition
({{!row.isEmpty() && (assumeLiveData || row.hasLiveData(nowInSec))}}) and the
second one ({{hasLiveStaticRow}})?
* In {{DataResolver}}, why not just calling {{rowCounted()}} unconditionally
(rather than adding helper methods)?
* In {{PkPrefixGroupMaker}}, instead of dealing with a ByteBuffer array, I'd
just kept the {{Clustering}} of the last row seen (null initially), and the
prefix size. We can then even delegate the check of whether the 2 clustering
belong to the same group to some {{Clustering}} method. On that front, we
shouldn't use {{ByteBuffer.equals}} but rather the {{ClusteringComparator}}
types. It happens that some types equality does not align with the bytes
equality (ok, one type, IntegerType, which allow as many zeroed bytes as prefix
without changing the actual value).
* In {{DataLimits.CQLGroupByLimits}} the silent assumption that {{filter()}}
should only called on replica, and {{newCounter()}} on the coordinator, feels
pretty error-prone for the caller (I haven't even carefully checked it's the
case tbh). One option would be to rename the methods to {{filterOnReplica()}}
and {{newCounterOnCoordinator()}} but even that feels a bit weird/limiting. I
would prefer removing the {{onReplica}} flag completely and handling that in
deserialization: we can assume when deserializing a {{DataLimits}} that we are
on the {{replica}} (it's arguably still a somewhat silent assumption, but one
that feels safer to me; we have no reason to serialize a {{ReadCommand}} except
to send it to replica, while having a need to call {{filter()}} on the
coordinator or {{newCounter}} on replicas in the future sounds very plausible),
so we could just call some {{withoutClustering()}} method on the {{State}} if
it's a range query. Another more explicit alternative could be to add some
{{onReplica()}} method to {{ReadCommand}} that we'd call in
{{ReadCommandVerbHandler}} (where we know for sure we are on a replica) that
would return a modified {{ReadCommand}} that would have a modified
{{DataLimits}}. That said, it's probably a bit more code just for that, so I
think I'm personally fine with the deserializer option.
* In {{GroupByAwareCounter}}:
** in {{applyToPartition}}, in the comment starting by {{partitionKey is the
last key for which we're returned rows}}, I believe it should read
{{state.partitionKey() is the last ...}}.
** at end of {{applyToPartition}}, why the {{!isDone()}} test? It seems that
since you just changed the
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15262117#comment-15262117
]
Benjamin Lerer commented on CASSANDRA-10707:
I pushed a new patch that should fix the short read logic and that add support
for {{PER PARTITION LIMIT}} with {{GROUP BY}}. The patch also add extra tests
for range name queries.
|[c* branch|https://github.com/blerer/cassandra/tree/10707-trunk]|[dtest
branch|https://github.com/riptano/cassandra-dtest/compare/master...blerer:CASSANDRA-10707?expand=1]|[utests|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-testall/]|[dtests|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-dtest/]|
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15179992#comment-15179992
]
Brian Hess commented on CASSANDRA-10707:
-
A quick question/clarification on the GROUP BY and ORDER BY discussion from
above. The following are valid SQL:
{code:SQL}
SELECT pkey, ccol1, Max(x) As maxx FROM myTable GROUP BY pkey, ccol1 ORDER BY
ccol1 pkey;
SELECT pkey, ccol1, Max(x) As maxx FROM myTable GROUP BY pkey, ccol1 ORDER BY
maxx;
{code}
I think you are suggesting that the only real ordering that is allowed is the
native ordering in the CQL tables. Specifically, these 2 queries would not be
supported. Is that correct?
I think the logic is more like that the following CQL
{code:SQL}
SELECT pkey, ccol1 Max(x) AS maxx FROM myTable GROUP BY pkey, ccol1 ORDER BY
pkey, ccol1
{code}
turns into
{code:SQL}
SELECT pkey, ccol1, Max(x) AS maxx FROM (
SELECT pkey, ccol1, x FROM myTable ORDER BY pkey, ccol1
) AS sub1 GROUP BY pkey, ccol1;
{code}
That means that the ORDER BY clause must work in that inner query as valid CQL.
More generally:
{code:SQL}
SELECT [grouping columns], [aggregate function]([aggregate columns]) FROM
[table] GROUP BY [grouping columns] ORDER BY [ordering columns]
{code}
Must satisfy the transformation to:
{code:SQL}
SELECT [grouping columns], [aggregate function]([aggregate columns]) FROM (
SELECT [grouping columns], [aggregate columns] FROM [table] ORDER BY
[ordering columns]
) AS sub1 GROUP BY [grouping columns]
{code}
And specifically that the inner sub-query is valid CQL, namely:
{code:SQL}
SELECT [grouping columns], [aggregate columns] FROM [table] ORDER BY [ordering
columns]
{code}
That is certainly different than SQL, which does not have this restriction.
I'm +0.5 on having the syntax be the same as SQL as I think it is slightly
better than the alternative. I'm just noting that the semantics really are a
bit different and there are more restrictions with the ORDER BY clause in CQL
with this ticket than in SQL. That nuance needs to be called out in the
documentation or folks will certainly run into the error.
I would also add that if someone uses an incorrect ORDER BY, the error should
not only call out that it is an error, but also indicate what sorts of ORDER BY
clauses are supported.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15179895#comment-15179895
]
Benjamin Lerer commented on CASSANDRA-10707:
||patch||utests||dtests||
|[trunk|https://github.com/apache/cassandra/compare/trunk...blerer:10707-trunk]|[trunk|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-testall/]|[trunk|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-dtest/]|
I have pushed new commits to handle the last review comments.
The patch does not fix yet the problem of shortRead but the current
implementation is definitely broken. As {{countedInCurrentPartition()}} is not
overridden the row count is used to compute the number of rows to fetch but the
limit is then set in term of groups.
I have to take something to properly investigate this problem.
{quote}In the news file, you have IN restrictions with only one element are now
considered as equality restrictions. What does that mean for the user?{quote}
Some queries which use to be rejected will suddenly get accepted like
multipartitions-queries with {{ORDER BY}} and paging.
Some error messages will also be slightly different.
I removed the entry for now as I agree that it might not be fully relevant for
the users.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15179738#comment-15179738
]
Benjamin Lerer commented on CASSANDRA-10707:
I missed the problem of the counted. Yes, I agree with your statement the short
read logic will probably not work for group by.
I will look deeper in it.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15179733#comment-15179733
]
Benjamin Lerer commented on CASSANDRA-10707:
{quote}I noticed that CQLLimits.forShortReadRetry() does not provide any limit
on the number of rows either.{quote}
It looks like I was really tired when I looked at the code :-(
{quote}Not sure about CQLGroupByLimits.forShortReadRetry(). I believe putting
no limit on the number of rows (and only on the group) might lead to OOM. In
fact, I need to think more carefully about this but I'm not 100% sure that the
short read logic isn't throw off by the fact that counted() returns a number of
groups not rows.{quote}
I think my implementation for {{forShortReadRetry()}} is simply wrong. The goal
of the short read retry is to fetch the rows that were missing for a given
partition due to short read. As the number of rows to request is computed in
{{ShortReadRowProtection:: moreContents}} I believe that even in the case of
{{GROUP BY}} we should use a {{CQLLimits}} to request the rows.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15173551#comment-15173551
]
Sylvain Lebresne commented on CASSANDRA-10707:
--
bq. The first point that needs discussing is that this patch changes the
inter-node protocol and this without bumping the protocol version
We had some offline discussion about this, and the consensus seems to be that
we'll leave it as is for this patch with just a fat warning in the NEWS file
that you shouldn't use {{GROUP BY}} until you've fully upgraded. As said above,
this is not perfect if someone don't follow that arguably intuitive instruction
but this'll do for this time. In the meantime, we'll fix the protocol
deserialization so it doesn't drop the connection if a message has more than it
expects, but just skips the message remainder. Longer term, we should introduce
at least major and minor versioning for the messaging protocol so we can deal
with this in a better way.
bq. the operation (filtering and ordering) commute. That's not really the case
for {{ORDER BY}} and {{GROUP BY}}.
Actually, I guess the grouping itself commutes, it's more the aggregation that
depend on the order. So nevermind, I'm good with sticking to the SQL syntax.
bq. Having a GroupMaker implementation for normal queries simplify the code as
the same algorythm can be used for the 3 scenarios.
I read the code too quickly, sorry, but still, I meant to go the same route
than for {{GroupSpecification.NO_GROUPING}}. The naming is equally confusing
imo and the code simplification is pretty detatable: we reference that
{{GroupMaker}} in {{Selection}} (only place where {{NO_GROUPING}} can be used I
believe) twice, so using some {{groupMaker != null}} won't make a big
difference.
bq. Even if we allow functions the {{lastClustering}} will always be a the last
clustering.
Fair enough, though I still feel like grouping it with the partition key in a
{{GroupMaker.State}} would be a tad cleaner. And at the very least, why use a
{{ByteBuffer[]}} for the clustering instead of {{Clustering}} which is a more
explicit?
bq. I tried the approach that you suggested but without success
I'll try to have a look in the coming days because I do feel it would be
cleaner and it ought to be possible but ...
bq. Performing the modification outside of the {{CQLGroupByLimits}} is probably
possible but will force us to modify the {{DataLimits}} and {{QueryPager}}
interfaces to expose the {{rowCount}}.
... I might be misunderstanding what this imply but that doesn't sound
particularly bad to me.
A few other remarks:
* In the news file, you have {{IN restrictions with only one element are now
considered as equality restrictions}}. What does that mean for the user?
* Could remove {{CFMetadaData.primaryKeyColumns()}} now that it's unused.
* The comment in {{DataLimits.CQLLimits.hasEnoughLiveData}} still misses some
part, it used to (and should) read {{Getting that precise _number forces_ us
...}}.
* Forgot that initially, but in {{DataLimits.CQLGroupByLimits.forPaging(int,
ByteBuffer, int)}}, it's fishy to me that we use the partition key in
parameters but reuse the pre-existing {{lastClustering}}. If we're guaranteed
than {{lastReturnedKey == lastPartitionKey}} then we should assert it as it's
not immediatly obvious, otherwise this is wrong.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15169740#comment-15169740
]
Benjamin Lerer commented on CASSANDRA-10707:
I have pushed a set of commits to address the different review points.
I have not changed the CQL syntax so far.
I did not rename {{CqlGroupByLimits}} as it is only used for {{GROUP BY}}
queries and not for aggregate queries (queries with aggregates but no group
by). I also did not rename {{GroupMaker}} as the class is really about creating
groups, but I am fully open to discussion.
I noticed that {{{CQLLimits.forShortReadRetry()}} does not provide any limit on
the number of rows either.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15169706#comment-15169706
]
Benjamin Lerer commented on CASSANDRA-10707:
[~slebresne] I tried the approach that you suggested but without success.
My idea was to increase the {{groupCount}} in the {{onPartitionClose()}} method
if the {{rowCount}} was not equals to the {{rowLimit}}. Unfortunatly, in the
case where the {{rowCount}} equals the {{rowLimit}} the next call return an
empty {{PartitionIterator}}. In this case the {{Counter}} does not receive any
notification and the pager consider that the data are exhausted. Without
callbacks it is difficult to adjust the {{groupCount}}. Performing the
modification outside of the {{CQLGroupByLimits}} is probably possible but will
force us to modify the {{DataLimits}} and {{QueryParser}} interfaces to expose
the {{rowCount}}.
I guess it is the reason why I ended up going in the direction of passing an
extra row up to the coordinator.
I do not deny that there might be a better way of doing the things but so far I
have not found it.
If you have some suggestions do not hesitate.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15167513#comment-15167513
]
Jon Haddad commented on CASSANDRA-10707:
I don't think changing the order of ORDER BY and GROUP BY is self explanatory,
so it doesn't really offer any benefit, imo. If I was trying out the feature
I'd mostly be annoyed by it's difference from something I've got muscle memory
for.
If you wanted to be technically accurate about it, SQL is declarative. The
order in which you specify the clauses doesn't matter, it just happens to line
up with how we mentally process it. If you chance the order of predicates in
your WHERE clause it doesn't matter, you'll still end up with the same query
result.
Assuming I'm understanding the implementation correctly, what you're saying is
that the query behaves more the following:
{code}
select * from
( select * from table order by some_field limit 100)
group by x,y,z
{code}
Is this correct, or am I missing something? If it's the case, I hope this
doesn't box us in later down the line if we want to add support for other
operations (like sub queries). If we're going to introduce more
inconsistencies with SQL (which may be totally fair, I'm just thinking out loud
here), we would want to put the GROUP BY after the LIMIT, since it's being
applied then. I'm not sure what this does to CQL in general, as now we've
implicitly made the decision to introduce clauses in an imperative fashion.
I'd rather not see new clauses added piece by piece with different rules
depending on the context, that definitely won't make things any easier.
So my question is, is CQL a declarative language or not? Will this ever be
something we intend to allow:
{code}
select username, score, state count(state) as c from top_scores where game_id=5
limit 1000 group by state order by c desc limit 5;
{code}
I don't think the above query works at all. The aggregation is clearly a
declarative clause.
Now, if the behavior of limit before aggregation is the right decision, that I
might have to argue with.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15167462#comment-15167462
]
Sylvain Lebresne commented on CASSANDRA-10707:
--
bq. we filter the data once they have been sorted, in most of the cases, but
the restrictions appear before the {{ORDER BY}} clause
The difference is that this has no external visibility because the operation
(filtering and ordering) commute. That's not really the case for {{ORDER BY}}
and {{GROUP BY}}. Suppose I take an aggregate that just return the first value
is has seen, you won't get at all the same result if you order before or after
you group. And I do think having the operation in the "wrong" order in the
syntax will be unintuitive in that case, and I'll admit I'm always for
privileging intuitiveness over sticking to familiarity with SQL. Opinion that
may or may not be popular, but I will add that I'm rather strongly against
forcing similarity with SQL if it goes against CQL self-consistency.
bq. If we consider that the pager is not exhausted if groupCount = groupLimit -
1 and rowCount < rowLimit, the user might end up requesting the next page for
nothing.
To hopefully help clarity I'll use "external" paging to describe the pages of
groups, the one sent to the user versus "internal" paging. I'm indeed
suggesting that 1) if replica hit the external page count (groupCount ==
groupLimit) then it doesn't send the first row of the next group and 2) on the
coordinator, we don't consider things exhausted if we hit exactly the external
page count.
Conceptually, this means that if an external page is complete, we assume there
is probably more (which may not be true). Note that this is the same logic that
normal paging use, so I'd argue that it's intuitive.
You approach consists in saying that when we do have a complete external page,
we send an additional row so the coordinator can decide whether or not we're
done or not.
My suggestion means we'll be wrong when the total number of groups for a query
is a multiple of the page size (and there is less groups than the user limit)
and will query one last empty page in the end in that case.
Your suggestion avoid this case but at the cost of
# transferring an extra row from replicas to coordinator for every "complete"
external page
# imo less clean and intuitive code
I suppose we can debate which of "an empty page query when #(groups) %
page_size == 0" and "an additional row transfered for every complete external
page" is worth, and we can definitively build examples where each one is better
than the other. I could argue however that, for a given user query, my
suggestion can only ever be worth than yours by some (small) constant factor
(the cost of an empty page), while if you take lots of groups, a smallish page
size and fat rows, your suggestion can be arbitrarily worth.
In practice, I suspect this doesn't matter much. Online queries almost never
page (if they do, you either don't care about performance or you'd better
configure your page size correctly) so both approach are the same. And
analytical queries are long enough that one additional empty page query don't
matter much, and if anything the "one additional row per complete page" might
matter more (though of course, rows are rarely too fat).
So to sum it up, I don't think any approach performance drawbacks matter for
the common case, but your solution has worst worst-cases and is, I argue, a bit
less clear code wise. I could still be missing something though and I apologize
if that's the case, but if I don't, I suspect you've guess which one I prefer :)
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15167398#comment-15167398
]
Jon Haddad commented on CASSANDRA-10707:
{code}
I do not think that the order in which we perform the operations and the CQL
syntax should be linked. If I am not mistaken, we filter the data once they
have been sorted, in most of the cases, but the restrictions appear before the
ORDER BY clause. In my opinion we should stick to the SQL syntax for that. As
most of the C* users have a SQL background it will prevent some confusions.
{code}
I agree with this, sticking with SQL syntax will trip people up a lot less.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15167259#comment-15167259
]
Benjamin Lerer commented on CASSANDRA-10707:
{quote}In the parser, we have the {{GROUP BY}} before the {{ORDER BY}}. On the
one side it's more consistent with {{SQL}} but on the other side, we do happen
to order before we group. And we kind of know that post-query ordering is not
really something we can do due to paging (we do happen to do it in a few case
when paging is not involved out of backward compatibility but given the
limitation, I don't think we'll want to ever extend that) so it does feel the
{{ORDER BY}} should come before {{GROUP BY}}.{quote}
I do not think that the order in which we perform the operations and the CQL
syntax should be linked. If I am not mistaken, we filter the data once they
have been sorted, in most of the cases, but the restrictions appear before the
{{ORDER BY}} clause. In my opinion we should stick to the {{SQL}} syntax for
that. As most of the C* users have a {{SQL}} background it will prevent some
confusions.
[~rustyrazorblade], [~pmcfadin] any opinion on the subject?
{quote}The {{forPagingByQueryPager}} and {{forGroupByInternalPaging}} methods
are not very intuitive: it's unclear when just looking at {{DataLimits}} why
they exist and why you should use them or not. I'm also not entirely sure we
need the difference between the two. If I understand correctly, the problem is
that when a replica ends a page in the middle of a group, the coordinator don't
know if said replica has stopped because it reaches the end of a group which
was the last one to return (in which case we're done), or if the replica
stopped because of the page limit (but the group itself is not finished). But
the coordinator should be able to figure that out by checking it's row counts:
if after having consumed the page from the replica we've reached the group
limit _or_ there was less row in the page than the page limit, then we're done,
otherwise we should ask for more. Granted, that does mean in the rare case
where we've exhausted all data exactly on the page limit, we'll do an
additional query (that will be empty), but pretty sure you have that problem in
one form or another whatever you do. Am I missing something here?{quote}
Let's take an example. If we have a table with one partition key {{a}}, 2
clustering columns {{b}} and {{c}} and the following data:
{code}
a | b | c
---+---+---
1 | 0 | 0
1 | 0 | 1
1 | 1 | 0
1 | 2 | 1
1 | 2 | 2
{code}
If we group on the column {{a}} and {{b}} with a page size of 2.
The 2 first rows will be returned in by the first internal page (rowCount = 2
and groupCount = 0 as we have not reached the end of the group).
The second internal page will read 2 rows: (1, 1, 0) and (1, 2, 1). When (1, 2,
1) will be reached, we will know that we have enough data to return to the
user. As (1, 2, 1) is not part of a group that should be returned to the user
it needs to be filtered out. If the row is filtered out on the replica the
{{GroupByLimits}} of the coordinator will only have a rowCount = 1 and a
groupCount = 1 and will assume that it has reached the end of the data (pager
exhausted).
If we consider that the pager is not exhausted if groupCount = groupLimit - 1
and rowCount < rowLimit, the user might end up requesting the next page for
nothing. I also believe that it will break paging if the page size is equals to
1 (that part I might have to check).
{quote} I feel keeping/passing the {{lastPartitionKey}} and {{lastClustering}}
in {{DataLimits.CQLGroupByLimits}} and {{GroupMaker}} is not as future
proof/generic as the patch is otherwise trying to be. Typically, if we allow
functions, the {{lastClustering}} won't really be a clustering. Granted, it'll
still fit into a {{ByteBuffer[]}} but it'll be a bit of an abuse. So I would
suggest adding {{GroupMaker.State}} concept whose implementation would be
specific to the {{GroupMaker/GroupBySpecification}} but would be transparent to
other code. Imo that would make some code cleaner (for instance,
{{GroupMaker.GROUP_EVERYTHING}} wouldn't need to store anything, which makes
sense).
.{quote}
We probably do not have the same design in mind. Even if we allow functions the
{{lastClustering}} will always be a the last clustering. The function will only
be applied within the {{GroupMaker}}.
{quote} I'm also confused as to why {{GroupMaker.NO_GROUPING}} even exists, we
shouldn't ever create a {{GroupMaker}} in the first place if we're not
aggregating at all.{quote}
When we build the {{ResultSet}} we need to handle 3 scenarios: normal query,
aggregate query (no group by) and group by query. Having a {{GroupMaker}}
implementation for normal queries simplify the code as the same algorythm can
be used for the 3 scenarios. As the {{GroupMaker.NO_GROUPING}} is a
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15163608#comment-15163608
]
Sylvain Lebresne commented on CASSANDRA-10707:
--
The first point that needs discussing is that this patch changes the inter-node
protocol and this without bumping the protocol version, which is something
we've never done.
On the one side bumping the protocol version is something we've kept for major
releases and so far planned not to do before 4.0. But on the other side, not
being able to add anything to the inter-node messages limits a lot new features
(like this one) and doesn't make sense in a tick-tock world.
We could accept this kind of change without bumping the version on the argument
that it's only "additive" to the serialization format and that it breaks
nothing if the new feature is not used, and we'd document that "you shall not
use GROUP BY until all nodes in the cluster are updated to a version that
supports it". But it's not something to treat lightly because if a user don't
respect that rule, older node will fail deserialization of the message, which
will drop the connection, which can impact other on-ongoing queries, which is
not good.
Alternatively, we could simply bump the protocol version, forgetting the "not
before 4.0" rule. The advantage is that if we do so we can have the sending
node (instead of the receiving one) throw if a GROUP BY is used but not all
nodes are up to date, which is cleaner and avoid the connection dropped
problem.
So something we need to decide first, and maybe we need a separate ticket to
have that discussion.
Anyway, outside of this question, some general remarks on the patch:
* In the parser, we have the {{GROUP BY}} before the {{ORDER BY}}. On the one
side it's more consistent with {{SQL}} but on the other side, we do happen to
order before we group. And we kind of know that post-query ordering is not
really something we can do due to paging (we do happen to do it in a few case
when paging is not involved out of backward compatibility but given the
limitation, I don't think we'll want to ever extend that) so it does feel the
{{ORDER BY}} should come before {{GROUP BY}}.
* The {{forPagingByQueryPager}} and {{forGroupByInternalPaging}} methods are
not very intuitive: it's unclear when just looking at {{DataLimits}} why they
exist and why you should use them or not. I'm also not entirely sure we need
the difference between the two. If I understand correctly, the problem is that
when a replica ends a page in the middle of a group, the coordinator don't know
if said replica has stopped because it reaches the end of a group which was the
last one to return (in which case we're done), or if the replica stopped
because of the page limit (but the group itself is not finished). But the
coordinator should be able to figure that out by checking it's row counts: if
after having consumed the page from the replica we've reached the group limit
_or_ there was less row in the page than the page limit, then we're done,
otherwise we should ask for more. Granted, that does mean in the rare case
where we've exhausted all data exactly on the page limit, we'll do an
additional query (that will be empty), but pretty sure you have that problem in
one form or another whatever you do. Am I missing something here?
* Not sure how I feel about {{GroupSelector}}. It's a bit confusing as of this
patch and while I get that its for allowing functions later, it might not be as
generic as we may need. For instance, if we ever want to support function on
multiple columns (which would be possible, assuming those are consecutive PK
columns), this won't work, at least not as implemented. I think I'd slightly
prefer removing {{GroupSelector}} for now and just rely on extending
{{GroupBySpecification}} for future extension (typically we'll add a
{{withFunctionGroupBySpec}} which would have all liberty as to how it is
implemented). Doing so also mean we can simplify the probably common and only
currently supported case as we just need to keep the length of the prefix we
group by rather than a list of (dumb) selector.
* I feel keeping/passing the {{lastPartitionKey}} and {{lastClustering}} in
{{DataLimits.CQLGroupByLimits}} and {{GroupMaker}} is not as future
proof/generic as the patch is otherwise trying to be. Typically, if we allow
functions, the {{lastClustering}} won't really be a clustering. Granted, it'll
still fit into a {{ByteBuffer[]}} but it'll be a bit of an abuse. So I would
suggest adding {{GroupMaker.State}} concept whose implementation would be
specific to the {{GroupMaker/GroupBySpecification}} but would be transparent to
other code. Imo that would make some code cleaner (for instance,
{{GroupMaker.GROUP_EVERYTHING}} wouldn't need to store anything, which makes
sense).
* {{GroupBySpeccification.NO_GROUPING}} feels
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15112398#comment-15112398
]
Benjamin Lerer commented on CASSANDRA-10707:
This ticket add only support for a {{GROUP BY}} clause with primary key columns.
We might add support for some specific functions in the {{GROUP BY}} clause.
Adding support for UDFs in the {{GROUP BY}} clause is a more tricky problem.
By grouping the data by primary key prefixes, we make sure that we control the
amount of memory used. As long as a function guaranty the same order as the
underlying column, it will work fine in the {{GROUP BY}} clause. If not, the
results returned will not make sense anymore. By consequence, allowing UDF in
the Group By clause present some risks.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15111571#comment-15111571
]
Brian Hess commented on CASSANDRA-10707:
-
Can a UDF be used as the GROUP BY key, such as:
SELECT partitionKey, clusteringCol1, Udf(clusteringCol2), Max( x ) FROM myData
WHERE partitionKey=5 GROUP BY partitionKey, clusteringCol1, Udf(clusteringCol2);
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15108647#comment-15108647
]
Benjamin Lerer commented on CASSANDRA-10707:
||patch||utests||dtests||
|[trunk|https://github.com/apache/cassandra/compare/trunk...blerer:10707-trunk]|[trunk|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-testall/]|[trunk|http://cassci.datastax.com/view/Dev/view/blerer/job/blerer-10707-trunk-dtest/]|
The DTest branch is
[here|https://github.com/riptano/cassandra-dtest/pull/753/files]
The classes {{GroupBySpecification}} and {{GroupSelector}} are used to create
{{GroupMaker}} instances. A {{GroupMaker}} is used, on a sorted set of rows, to
determine if a row belongs to the same group as the previous row or not.
For the moment, only one type of {{GroupSelector}} exists for a primary key
columns. Its serialization mechanism has, nevertheless, been implemented in
such a way that it will be possible to add new implementations (to allow the
use of functions in the {{GROUP BY}} clause, for example) without breaking
backward compatibility.
{{SelectStatement}} and {{Selection}} have been modified in order to use
{{GroupBySpecification}} and {{GroupMaker}} when building the result set.
Group by queries are always paged internally to avoid {{OOMExceptions}}. Two
new {{DataLimits}} have been added to manage the group by paging
{{CQLGroupByLimits}} and {{CQLGroupByPagingLimits}}. They keep track of the
group count and of the row count to make sure that the processing is stopped as
soon as one of the limits is reached.
A group is only counted once the next one is reached, as a group can be spread
over multiple pages. The problem with this approach is that a counter can only
know if it has reach the group limit when it has reached a row that should not
be added to the resultset. As multiple counters are used when a request is
processed the extra row is not filtered out until it reachs the counter of the
{{QueryPager}}. To do that a special factory method has been added to
{{DataLimits}}: {{forPagingByQueryPager(int pageSize)}}.
This approach was not working properly in the case of the
{{MultiPartitionPager}} as an extra counter was added on top of the one of the
{{SinglePartitionPager}}. To solve that problem the use of the counter in
{{MultiPartitionPager}} has been replaced by another mechanism.
The internal paging is performed by the {{GroupByQueryPager}} which
automatically fetch new pages of data when needed.
As the {{DataLimits}} needs to be updated for each new internal query and the
{{ReadQuery}} classes are immutable a new {{withUpdatedLimit}} method as been a
added to all the {{ReadQuery}} classes.
In order to simplify the {{SelectStatement}} code, the patch also modify
slightly the way queries with aggregates but no {{GROUP BY}} is working.
I implemented it initially on top of the Group by paging but realized afterward
that it was breaking backward compatibility. We will anyway be able in the
future to switch back to it. Once we are sure that the group by paging is
supported by the previous versions.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15108848#comment-15108848
]
Aleksey Yeschenko commented on CASSANDRA-10707:
---
bq. In the future, we might manage to push the aggregate computation to the
replicas but we are not there yet.
I think what Brian meant here wasn't pushing computation to replicas, but
splitting it between sub-coordinators.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15109181#comment-15109181
]
Brian Hess commented on CASSANDRA-10707:
-
Correct, what [~iamaleksey] said. In fact, pushing the aggregate computation
to the replicas is troublesome at an RF>1.
Quick follow up - will this ticket also cover:
SELECT clusterCol, Max(x) FROM myData GROUP BY clusterCol;
That is, you group on a clustering column, but not on a partition key?
Second question - consider a table with schema myData(partitionKey INT,
clusteringCol1 INT, clusteringCol2 INT, x INT, PRIMARY KEY ((partitionKey),
clusteringCol1, clusteringCol2). Now, will the following query be supported:
SELECT partitionKey, clusteringCol2, Sum(x) FROM myData GROUP BY partitionKey,
clusteringCol2;
The reason I ask is that the following is not supported:
SELECT partitionKey, clusteringCol2, x FROM myData WHERE partitionKey=5 ORDER
BY clusteringCol2;
Because you cannot order by clusteringCol2, only clusteringCol1.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15109333#comment-15109333
]
Benjamin Lerer commented on CASSANDRA-10707:
Those queries will not be supported by this ticket.
If such a query is required, it is possible to use a materialized view to
reorganized the data in order to allow the group by query.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15108289#comment-15108289
]
Benjamin Lerer commented on CASSANDRA-10707:
Taking into account the fact that we allow today queries like: {{SELECT
max((*)), min((*)), count((*)) FROM myTable;}}, I do not really think that we
can provide support for {{GROUP BY}} queries without allowing to group by
partition keys.
Some users might be interested by queries like: {{SELECT max((*)), min((*))
count((*)) FROM myTable GROUP BY partitionKey;}} or {{SELECT max((*)), min((*))
count((*)) FROM myTable WHERE partitionKey IN (1, 2, 3) GROUP BY partitionKey;}}
Now, it is clear that those queries are not recommended and that the timeouts
will probably need to be adjusted. As for the current aggregates queries a
warning will be logged to warn the users if the partition key is not restricted
by an equality.
The problem is not really the work needed to compute the aggregates. It is just
the fact that the data has to be retrieved from other nodes.
In the future, we might manage to push the aggregate computation to the
replicas but we are not there yet.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15107583#comment-15107583
]
Brian Hess commented on CASSANDRA-10707:
-
I think that supporting grouping by clustering column (or perhaps even a
regular column) with a partition key predicate is a good idea.
I think that supporting grouping by partition key (either in part, or in toto)
is a bad idea. In that query, all the data in the cluster would stream to the
coordinator who would then be responsible for doing a *lot* of processing. In
other distributed systems that do GROUP BY queries, the groups end up being
split up among the nodes in the system and each node is responsible for rolling
up the data for those groups it was assigned. This is a common way to get all
the nodes in the system to help with a pretty significant computation - and the
data streamed out (potentially via a single node in the system) to the client.
However, in this approach, all the data is streaming to a single node and that
node is doing all the work, for all the groups. This feels like either a ton
of work to orchestrate the computation (that would start to mimic other systems
- e.g., Spark) or would do a lot of work and risk being very inefficient and
slow. I am also concerned to what this would do in the face of
QueryTimeoutException - would we really be able to do a GROUP BY partitionKey
aggregate under the QTE limit?
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15076378#comment-15076378
]
Benjamin Lerer commented on CASSANDRA-10707:
Both will be supported.
What will not be supported is a {{group by}} clause were only a part of the
partition key will be specified. For example, if a table has a primary key like
{{PRIMARY KEY((partitionKey1, partitionKey2) clustering1, clustering2)}}, the
following query will not be supported:
{{SELECT partitionKey1, MAX(value) FROM myTable GROUP BY partitionKey1}}
As for the aggregates, the grouping will be performed on the coordinator node.
By consequence, if the driver use the Token aware policy, a query containing a
partition key predicate will be more efficient as the aggregates will be built
on the node where the data are located.
>From the syntax point of view, the queries:
{{SELECT partitionKey, clusteringColumn1, Max(value) FROM myTable WHERE
partitionKey=5 GROUP BY partitionKey, clusteringColumn1;}}
and {{SELECT partitionKey, clusteringColumn1, Max(value) FROM myTable WHERE
partitionKey=5 GROUP BY clusteringColumn1;}} will be both supported due to the
fact that the {{partitionKey}} column is restricted by an {{=}} operator.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15076099#comment-15076099
]
Brian Hess commented on CASSANDRA-10707:
-
[~blerer] - will this GROUP BY require a partitionKey predicate? That is, will
the following be supported:
SELECT partitionKey, clusteringColumn1, Max(value) FROM myTable GROUP BY
partitionKey, clusteringColumn1;
Or will it also require a partitionKey predicate like:
SELECT partitionKey, clusteringColumn1, Max(value) FROM myTable WHERE
partitionKey=5 GROUP BY partitionKey, clusteringColumn1;
Or will both be supported?
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-10707) Add support for Group By to Select statement
[
https://issues.apache.org/jira/browse/CASSANDRA-10707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15066337#comment-15066337
]
Benjamin Lerer commented on CASSANDRA-10707:
The main difficulty of the ticket is the paging. Between the client and the
coordinator nodes the page are returned based on the grouping but internally
the data are paged by number of rows.
For example, if a {{Group by}} query is used with a page size of 5000, the
first page returned to the client must contains the aggregates for the first
5000 groups or less (if there was less than 5000 groups). As these groups can
be composed of a big number of rows, in order to avoid OOM errors, the
coordinator node need to request pages of data from the other nodes until it
has enough groups. One of the problem being that it is only possible to be sure
that a group is complete when the next group is reached or the data exhausted.
> Add support for Group By to Select statement
>
>
> Key: CASSANDRA-10707
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10707
> Project: Cassandra
> Issue Type: Improvement
> Components: CQL
>Reporter: Benjamin Lerer
>Assignee: Benjamin Lerer
>
> Now that Cassandra support aggregate functions, it makes sense to support
> {{GROUP BY}} on the {{SELECT}} statements.
> It should be possible to group either at the partition level or at the
> clustering column level.
> {code}
> SELECT partitionKey, max(value) FROM myTable GROUP BY partitionKey;
> SELECT partitionKey, clustering0, clustering1, max(value) FROM myTable GROUP
> BY partitionKey, clustering0, clustering1;
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
