You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/06/16 16:24:11 UTC
[GitHub] [cassandra] adelapena opened a new pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
adelapena opened a new pull request #635:
URL: https://github.com/apache/cassandra/pull/635
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena closed pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
adelapena closed pull request #635:
URL: https://github.com/apache/cassandra/pull/635
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465218055
##########
File path: src/java/org/apache/cassandra/service/DataResolver.java
##########
@@ -904,9 +905,11 @@ public UnfilteredRowIterator moreContents()
return UnfilteredPartitionIterators.getOnlyElement(executeReadCommand(cmd), cmd);
}
- // Counts the number of rows for regular queries and the number of groups for GROUP BY queries
+ /** Returns the number of results counted in the partition by the counter */
private int countedInCurrentPartition(Counter counter)
{
+ // We are interested by the number of rows but for GROUP BY queries 'countedInCurrentPartition' returns
+ // the number of groups in the current partition.
return command.limits().isGroupByLimit()
? counter.rowCountedInCurrentPartition()
: counter.countedInCurrentPartition();
Review comment:
@adelapena Similar comment as above. `counter.rowCountedInCurrentPartition()` returns the same thing as `counter.countedInCurrentPartition()` in the non-grouping 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465795775
##########
File path: src/java/org/apache/cassandra/db/filter/DataLimits.java
##########
@@ -934,7 +934,7 @@ protected Row applyToStatic(Row row)
// It's possible that we're "done" if the partition we just started bumped the number of groups (in
// applyToPartition() above), in which case Transformation will still call this method. In that case, we
// want to ignore the static row, it should (and will) be returned with the next page/group if needs be.
- if (isDone())
+ if (enforceLimits && isDone())
Review comment:
Right, just added an in-JVM dtest for each condition.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465217710
##########
File path: src/java/org/apache/cassandra/service/DataResolver.java
##########
@@ -759,9 +759,10 @@ public UnfilteredPartitionIterator moreContents()
return executeReadCommand(cmd);
}
- // Counts the number of rows for regular queries and the number of groups for GROUP BY queries
+ /** Returns the number of results counted by the counter */
private int counted(Counter counter)
{
+ // We are interested by the number of rows but for GROUP BY queries 'counted' returns the number of groups.
return command.limits().isGroupByLimit()
? counter.rowCounted()
: counter.counted();
Review comment:
@adelapena Even if we can only do this in trunk (because of the Thrift counter in 3.11), why don't we just get rid of the `counted()` method altogether and call `counter.rowCounted()` (which I would rename to `rowsCounted()`, btw) in all cases? `rowCounted()` and `counted()` return the same thing in the non-grouping case anyway :/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465835874
##########
File path: src/java/org/apache/cassandra/service/DataResolver.java
##########
@@ -759,9 +759,10 @@ public UnfilteredPartitionIterator moreContents()
return executeReadCommand(cmd);
}
- // Counts the number of rows for regular queries and the number of groups for GROUP BY queries
+ /** Returns the number of results counted by the counter */
private int counted(Counter counter)
{
+ // We are interested by the number of rows but for GROUP BY queries 'counted' returns the number of groups.
return command.limits().isGroupByLimit()
? counter.rowCounted()
: counter.counted();
Review comment:
Makes sense, done for trunk.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] maedhroz commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465242986
##########
File path: src/java/org/apache/cassandra/db/filter/DataLimits.java
##########
@@ -934,7 +934,7 @@ protected Row applyToStatic(Row row)
// It's possible that we're "done" if the partition we just started bumped the number of groups (in
// applyToPartition() above), in which case Transformation will still call this method. In that case, we
// want to ignore the static row, it should (and will) be returned with the next page/group if needs be.
- if (isDone())
+ if (enforceLimits && isDone())
Review comment:
@adelapena It doesn't look like anything in `TestConsistency::test_group_by_srp` fails if we remove this. (i.e. It looks like the new logic is correct, just doesn't have any coverage.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a change in pull request #635: CASSANDRA-15459 3.11 Fix SRP on group-by queries
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #635:
URL: https://github.com/apache/cassandra/pull/635#discussion_r465795775
##########
File path: src/java/org/apache/cassandra/db/filter/DataLimits.java
##########
@@ -934,7 +934,7 @@ protected Row applyToStatic(Row row)
// It's possible that we're "done" if the partition we just started bumped the number of groups (in
// applyToPartition() above), in which case Transformation will still call this method. In that case, we
// want to ignore the static row, it should (and will) be returned with the next page/group if needs be.
- if (isDone())
+ if (enforceLimits && isDone())
Review comment:
Right, just added an in-JVM dtests for each condition.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org