STAR-762: Implement paging in bytes#223
Merged
jacek-lewandowski merged 28 commits intods-trunkfrom Jul 30, 2021
Merged
Conversation
jacek-lewandowski
force-pushed
the
STAR-762
branch
3 times, most recently
from
92be896 to
522a2c9
Compare
djatnieks
reviewed
Jul 23, 2021
djatnieks
reviewed
Jul 26, 2021
Member
djatnieks
left a comment
djatnieks
left a comment
There was a problem hiding this comment.
About 80-90% through all the changes; looks good so far. Want another round with the pagers to better understand them.
Current comments are minor suggestions/questions; wanted to provide some feedback along the way.
Member
|
Previous guardrails porting included most of the changes from DB-3208, but I believe at least some of the paging changes from this commit are still needed: https://github.com/riptano/bdp/pull/15713/commits/fd60634683cbab4878548263bbc484063a65ee0f e.g. changes in |
Author
|
@djatnieks I hope I've address all your comments |
djatnieks
requested changes
Jul 28, 2021
Member
djatnieks
left a comment
djatnieks
left a comment
There was a problem hiding this comment.
Overall looks good, and the recent refactor and comments helped to understand the paging stuff better in general; thanks for that extra effort!
A few more minor comments, and just these changes I'd request to consider (summarized from individual comments):
- Align
GuardrailPagingTestwith the latest onbdp/6.8-cndbbranch and use same package as other guardrail tests. - Fix failing
GuardrailPagingTest.testQueryWithSmallBytePagesWorks(suggested fix inSelectStatement)
Member
There was a problem hiding this comment.
Protect against currentPartitionKey == null?
I do not know usage well enough - asking out of caution.
djatnieks
approved these changes
Jul 29, 2021
Member
djatnieks
left a comment
djatnieks
left a comment
There was a problem hiding this comment.
One more comment to consider, otherwise good to go.
Member
There was a problem hiding this comment.
Should this also include && !pageSize.isCompleted(query.limits().bytes(), PageSize.PageUnit.BYTES)?
Author
There was a problem hiding this comment.
Actually, that previous change was wrong. So basically the check:
if (aggregationSpec == null && !pageSize.isCompleted(query.limits().count(), PageSize.PageUnit.ROWS))
return execute(query, options, queryState, selectors, nowInSec, userLimit, queryStartNanoTime);
should verify we paging should be applied at all. What we want to do there is to verify whether the requested page size is greater than the user provided limits. Say if the user specifies LIMIT 50 and the page size in rows is 100, then paging is unnecessary. However if the page size is provided in bytes, we cannot say anything about that. I've extracted that condition into a separate method and added a comment.
So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want
jacek-lewandowski
added a commit
that referenced
this pull request
Oct 17, 2022
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea)
jacek-lewandowski
added a commit
that referenced
this pull request
Oct 18, 2022
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea)
mfleming
pushed a commit
that referenced
this pull request
Jul 10, 2023
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2)
djatnieks
pushed a commit
that referenced
this pull request
Jul 24, 2023
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2)
djatnieks
pushed a commit
that referenced
this pull request
Aug 21, 2023
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0)
djatnieks
pushed a commit
that referenced
this pull request
Sep 12, 2023
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) STAR-762 Fix conflict in DunpTest with CASSANDRA-18215
jacek-lewandowski
added a commit
that referenced
this pull request
Jan 28, 2024
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849)
djatnieks
pushed a commit
that referenced
this pull request
Mar 29, 2024
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
djatnieks
pushed a commit
that referenced
this pull request
Apr 1, 2024
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
djatnieks
pushed a commit
that referenced
this pull request
Apr 16, 2024
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
djatnieks
pushed a commit
that referenced
this pull request
Jan 30, 2025
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
djatnieks
pushed a commit
that referenced
this pull request
May 18, 2025
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 6, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now.
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 10, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 11, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 12, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 14, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 16, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Feb 27, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Mar 2, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
michaelsembwever
pushed a commit
that referenced
this pull request
Mar 4, 2026
Add page size in bytes flag to protocol Introduce PageSize object Protocol version changes No support for describe statement yet Simplify SecondaryIndexManager page calculation Add page size in bytes to DataLimits Refactor pagers Add / pull some tests Add some toString implementations Add PageSize to expected classes in DatabaseDescriptorRefTest Fix AggregationPartitionIterator So far we were passing the main page size to the AggregationPartitionIterator, which: - was pointless because there is no paging when we aggregate everything - it was actually harmful because AggregationPartitionIterator is a subclass of GroupByPartitionIterator and the later updates the subPager's limits with the minimum count of main page size and the number of remaining. It is correct if we use grouping aware limits, where count applies to the whole groups. But when we do aggregate everything, simply CQL limits are used and count limit applies to rows. Concluding, without fixing that we would limit the number of aggregated rows to the main page size which is not what we want (cherry picked from commit e11d716) (cherry picked from commit 13d4569) (cherry picked from commit 4f65564) (cherry picked from commit 9d084ea) (cherry picked from commit b0f05f2) (cherry picked from commit 8dd49b0) (cherry picked from commit 223d849) STAR-762 Fix PagingQueryTest with updated internal row size. STAR-762 Fix GuardrailPageWeightTest Align Guardrails and GuardrailsMBean page weight get/set warn/fail threshold methods with others that use DataStorageSpec values; STAR-762 Fix GuardrailPageWeightTest by removing code added during rebase that alters the data limit given by the user (e.g. LIMIT 100) to instead be the page size limit. Since that was different from existing CC or C* and broke the GuardrailPageWeightTest, is seems best to remove that for now. (Rebase of commit f19c921)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.