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.

Sign up for GitHub

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

Jump to bottom

Limit clause breaks query results #42509

Closed
Tracked by #40313
kamilmielnik opened this issue May 10, 2024 · 3 comments
Closed
Tracked by #40313

Limit clause breaks query results #42509

kamilmielnik opened this issue May 10, 2024 · 3 comments
Assignees
@camsaul
Labels
.Backend Querying/Processor .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects

Comments

@kamilmielnik
Copy link
Contributor

kamilmielnik commented May 10, 2024 •
edited

e2e repro in this PR: #42455 (test is skipped for now)

Repro steps:

  1. Create a question based on orders table
  2. Add custom expression aggregation: Offset(Sum([Total]), -1)
  3. Add a breakout by Orders > Created At: Month
  4. Add a breakout by Products > Category
  5. Visualize the query as a table - it works as expected (5 columns, 193 rows)
  6. Go back to notebook editor
  7. Add row limit 5
  8. Visualize the query

The results have only 4 rows and 3 columns and the table has been transposed compared to results in step 5.

It's expected that the results will look the same as in step 5, but limited to 5 first rows.
@kamilmielnik kamilmielnik mentioned this issue May 10, 2024
Closed
kamilmielnik added a commit that referenced this issue May 10, 2024
@kamilmielnik
Add a repro for #42509
656dd98
@camsaul camsaul self-assigned this May 10, 2024
@kamilmielnik kamilmielnik mentioned this issue May 13, 2024
Merged
kamilmielnik added a commit that referenced this issue May 13, 2024
@kamilmielnik
Cover Offset() in aggregations with e2e tests (#42455)
f596f0b 
* Add repro for 32323

* Fix offset not working in case

* Make offset function return any

* Add a repro for #42377

* Fix order of adjustments

* Revert unrelated changes

* Remove commented code

* Revert unrelated changes

* Refactor test

* Type offset aggregation

* Add a test for no order by or breakout clause

* Add a basic test for breakout clause

* Fix assertion

* Remove problematic dependency

* Fix uuids generation

* Remove redundant limit

* Add a test for multiple breakouts

* Update test names

* Extract OFFSET_SUM_TOTAL_AGGREGATION

* Add a repro for #42509

* Add a test for multiple aggregations and breakouts

* Remove unused intercept

* Move uuid utils to separate file
- it wasn't possible to import the utils file in e2e tests without it

* Make isUuid a type guard

* Add tests for sorting

* Tag the test

* Add extra assertion to verify expression parsing

* Add a complex scenario

* Reverse isFirst logic
@camsaul
Copy link
Member

camsaul commented May 16, 2024

Turns out query results are actually just fine, but the FE is visualizing things as a pivot table by default. Disabling pivot visualization shows you 5 rows

image

@camsaul
Copy link
Member

camsaul commented May 16, 2024

Note: you can easily verify that the QP returned 5 rows by looking at the response in the Network inspector

@camsaul
Copy link
Member

camsaul commented May 16, 2024

Closing as not a bug

See Slack discussion for more info https://metaboat.slack.com/archives/C06P22KS4JH/p1715820619367019

@camsaul camsaul closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2024
@kamilmielnik kamilmielnik mentioned this issue May 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@camsaul camsaul

Labels
.Backend Querying/Processor .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@camsaul @kamilmielnik