Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow filtering queries with aggregation that page returns intermediate aggregated results, not the full one #4540

Closed
glommer opened this issue Jun 11, 2019 · 15 comments
Assignees
Milestone

Comments

@glommer
Copy link
Contributor

@glommer glommer commented Jun 11, 2019

This is Scylla's bug tracker, to be used for reporting bugs only.
If you have a question about Scylla, and not a bug, please ask it in
our mailing-list at scylladb-dev@googlegroups.com or in our slack channel.

  • I have read the disclaimer above, and I am reporting a suspected malfunction in Scylla.

Our dear and esteemed friend @ultrabug hit today what he thought was Ultra Bug, but upon closer investigation seemed like just a variation of #4156

However, what makes his case unique is that he is doing aggregations:

scylla@cqlsh:test> select sum(ultra_column) as volume from ultrabug_table where ultra_column = 'some word in french' allow filtering;

 volume
--------
    476

---MORE---
 volume
--------
    496

---MORE---
 volume
--------
    496

---MORE---
 volume
--------
    480

---MORE---
 volume
--------
    488

---MORE---
 volume
--------
    496

The query works just fine if we disable paging, but we are both in agreement that since this is an aggregation, the sane user behavior would be that the aggregation would be done in the coordinator. In fact, since select count(*) from table seems to always time out, am I wrong to believe that in the case of normal queries, the aggregation happens in the coordinator already?

@nyh

This comment has been minimized.

Copy link
Contributor

@nyh nyh commented Jun 11, 2019

I wonder if this is related to issue #4127, which has a similar problem for count(*)?

@avikivity

This comment has been minimized.

Copy link
Contributor

@avikivity avikivity commented Jun 11, 2019

What's the bug? are the results incorrect?

@glommer

This comment has been minimized.

Copy link
Contributor Author

@glommer glommer commented Jun 11, 2019

The bug is that we'd both expect that when an aggregation is used, the final aggregation is returned, and not the result of the aggregation for every page.

Are our expectations wrong? Why ?

@avikivity

This comment has been minimized.

Copy link
Contributor

@avikivity avikivity commented Jun 11, 2019

Certainly a query with aggregation and without grouping should return a single result. It would be nice to state this explicitly in the report.

@glommer glommer changed the title paging and allow filtering affecting aggregations as well allow filtering queries with aggregation that page returns intermediate aggregated results, not the full one Jun 11, 2019
@glommer

This comment has been minimized.

Copy link
Contributor Author

@glommer glommer commented Jun 11, 2019

I changed the title.

@psarna

This comment has been minimized.

Copy link
Member

@psarna psarna commented Jun 12, 2019

Which Scylla version is that? I wrote a test for it on master and it passes seamlessly. Maybe it's a version pre- acf4ead, which, by accident, solved that problem as well?

psarna added a commit to psarna/scylla that referenced this issue Jun 12, 2019
The test case ensures that filtering with paging works fine if
an aggretation function is used - the results should be presented
as a single aggregated value, without partial results per-page.

Ref scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 12, 2019
The test case ensures that filtering with paging works fine if
an aggretation function is used - the results should be presented
as a single aggregated value, without partial results per-page.

Ref scylladb#4540
@ultrabug

This comment has been minimized.

Copy link
Contributor

@ultrabug ultrabug commented Jun 12, 2019

3.0.6 @psarna

@slivne slivne added this to the 3.2 milestone Jun 13, 2019
@psarna

This comment has been minimized.

Copy link
Member

@psarna psarna commented Jun 14, 2019

I just double checked on 3.0 and the test I added passes fine as well... which means the issue might still be here and it's more subtle. I'll investigate.

@psarna

This comment has been minimized.

Copy link
Member

@psarna psarna commented Jun 14, 2019

Alright, I figured it out. It's actually all about indexing + aggregation + paging, and the problem is as follows:

  1. In the original Scylla implementation of secondary indexes, paging was not supported.
  2. Later, paging support was added. It was a little bit more complicated than paging regular queries, because the paging state from the indexing query needs to be internally reforged into the paging state of the base table in order to keep fetching proper results.
  3. Unfortunately, the matter of aggregation queries was not taken into account when implementing 2.

The way regular aggregation works inside Scylla is that the requests are still internally paged (in order to avoid OOM) and aggregated on coordinator side. Such a mechanism is not yet present in SI.

One simple workaround is to disable paging for aggregation queries that use indexing, but it can be detrimental for huge result sets. The other is to gather partial results and combine them client-side, but that involves action from the client and assumes that the aggregation is commutative, which is true for sum and count, but less for avg.

The proper solution is to implement internal paging for aggregation indexed queries, based on the existing code for regular queries.

@eliransin @slivne let me know what's the priority of this. If it's not super high, it might be a decent n00b task to get acquainted with CQL layer.

@nyh

This comment has been minimized.

Copy link
Contributor

@nyh nyh commented Jun 17, 2019

If this bug also involves the secondary index, then issue #4127 (which we were never able to reproduce) is most likely a duplicate.

psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 17, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 24, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Jun 24, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
nyh added a commit that referenced this issue Jun 24, 2019
…ithub.com/psarna/scylla into next

Piotr Sarna says:

Fixes #4540
This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)

Piotr Sarna (8):
  cql3: split execute_base_query implementation
  cql3: enable explicit copying of query_options
  cql3: add a query options constructor with explicit page size
  cql3: add proper aggregation to paged indexing
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  tests: add query_options to cquery_nofail
  tests: add indexing + paging + aggregation test case
  tests: add indexing+paging test case for clustering keys
avikivity added a commit that referenced this issue Jun 25, 2019
…ithub.com/psarna/scylla into next

Piotr Sarna says:

Fixes #4540
This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)

Piotr Sarna (8):
  cql3: split execute_base_query implementation
  cql3: enable explicit copying of query_options
  cql3: add a query options constructor with explicit page size
  cql3: add proper aggregation to paged indexing
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  tests: add query_options to cquery_nofail
  tests: add indexing + paging + aggregation test case
  tests: add indexing+paging test case for clustering keys
avikivity added a commit to avikivity/scylla that referenced this issue Nov 13, 2019
…ithub.com/psarna/scylla into next

Piotr Sarna says:

Fixes scylladb#4540
This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)

Piotr Sarna (8):
  cql3: split execute_base_query implementation
  cql3: enable explicit copying of query_options
  cql3: add a query options constructor with explicit page size
  cql3: add proper aggregation to paged indexing
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  tests: add query_options to cquery_nofail
  tests: add indexing + paging + aggregation test case
  tests: add indexing+paging test case for clustering keys

(cherry picked from commit a88c9ca)
@avikivity

This comment has been minimized.

Copy link
Contributor

@avikivity avikivity commented Nov 13, 2019

@psarna please prepare backports against 3.1 and 3.0

( tried and failed, see my next-3.1-4540-backport branch)

psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
@psarna

This comment has been minimized.

psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
avikivity added a commit that referenced this issue Nov 14, 2019
"
Fixes #4540

This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)
"

* 'add_proper_aggregation_for_paged_indexing_for_3.1' of https://github.com/psarna/scylla:
  tests: add indexing+paging test case for clustering keys
  tests: add indexing + paging + aggregation test case
  tests: add query_options to cquery_nofail
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  cql3: add proper aggregation to paged indexing
  cql3: add a query options constructor with explicit page size
  cql3: enable explicit copying of query_options
  cql3: split execute_base_query implementation
@avikivity

This comment has been minimized.

Copy link
Contributor

@avikivity avikivity commented Nov 14, 2019

Queued for 3.1.

psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Aggregated and paged filtering needs to aggregate the results
from all pages in order to avoid returning partial per-page
results. It's a little bit more complicated than regular aggregation,
because each paging state needs to be translated between the base
table and the underlying view. The routine keeps fetching pages
from the underlying view, which are then used to fetch base rows,
which go straight to the result set builder.

Fixes scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
psarna added a commit to psarna/scylla that referenced this issue Nov 14, 2019
Indexed queries used to erroneously return partial per-page results
for aggregation queries. This test case used to reproduce the problem
and now ensures that there would be no regressions.

Refs scylladb#4540
@psarna

This comment has been minimized.

Copy link
Member

@psarna psarna commented Nov 14, 2019

I also did 3.0 right after, but forgot to post it... here it is: https://github.com/psarna/scylla/tree/add_proper_aggregation_for_paged_indexing_for_3.0

avikivity added a commit that referenced this issue Nov 17, 2019
"
Fixes #4540

This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)
"

* 'add_proper_aggregation_for_paged_indexing_for_3.1' of https://github.com/psarna/scylla:
  test: add 'eventually' block to index paging test
  tests: add indexing+paging test case for clustering keys
  tests: add indexing + paging + aggregation test case
  tests: add query_options to cquery_nofail
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  cql3: add proper aggregation to paged indexing
  cql3: add a query options constructor with explicit page size
  cql3: enable explicit copying of query_options
  cql3: split execute_base_query implementation
avikivity added a commit that referenced this issue Nov 17, 2019
"
Fixes #4540

This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)
"

* 'add_proper_aggregation_for_paged_indexing_for_3.0' of https://github.com/psarna/scylla:
  test: add 'eventually' block to index paging test
  tests: add indexing+paging test case for clustering keys
  tests: add indexing + paging + aggregation test case
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  cql3: add proper aggregation to paged indexing
  cql3: add a query options constructor with explicit page size
  cql3: enable explicit copying of query_options
  cql3: split execute_base_query implementation
@avikivity

This comment has been minimized.

Copy link
Contributor

@avikivity avikivity commented Nov 17, 2019

Queued for 3.0, 3.1

avikivity added a commit that referenced this issue Nov 19, 2019
"
Fixes #4540

This series adds proper handling of aggregation for paged indexed queries.
Before this series returned results were presented to the user in per-page
partial manner, while they should have been returned as a single aggregated
value.

Tests: unit(dev)
"

* 'add_proper_aggregation_for_paged_indexing_for_3.0' of https://github.com/psarna/scylla:
  test: add 'eventually' block to index paging test
  tests: add indexing+paging test case for clustering keys
  tests: add indexing + paging + aggregation test case
  cql3: make DEFAULT_COUNT_PAGE_SIZE constant public
  cql3: add proper aggregation to paged indexing
  cql3: add a query options constructor with explicit page size
  cql3: enable explicit copying of query_options
  cql3: split execute_base_query implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.