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

diagnose-expression throws when Offset is nested #42377

Closed
Tracked by #40313
kamilmielnik opened this issue May 8, 2024 · 2 comments · Fixed by #42326 or #42497
Closed
Tracked by #40313

diagnose-expression throws when Offset is nested #42377

kamilmielnik opened this issue May 8, 2024 · 2 comments · Fixed by #42326 or #42497
Assignees
@kamilmielnik
Labels
.Frontend Querying/GUI Query builder catch-all, including simple mode .Team/QueryingComponents Type:Bug Product defects
Milestone
0.50

Comments

@kamilmielnik
Copy link
Contributor

kamilmielnik commented May 8, 2024 •
edited

Repro steps:

  1. git checkout 42318-offset-custom-columns-frontend
  2. Start a new question based on Orders table
  3. Try to add a custom column with custom expression: case(Offset([Total], -1) > 5, "abc")
  4. Observe error in JS console
Error normalizing query: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Error normalizing form: Assert failed: (= (count clause) 4)

Screenshot
@kamilmielnik kamilmielnik mentioned this issue May 8, 2024
Open
@kamilmielnik kamilmielnik changed the title diagnose-expression throws when Offset is nested diagnose-expression throws when Offset is nested May 8, 2024
@camsaul
Copy link
Member

camsaul commented May 8, 2024

I'm guessing this is a bug with the FE code that parses case from user input into an actual MBQL clause, it's probably not recursively calling the same parsing code on its arguments. offset needs to go thru code that adds the options map automatically

@kamilmielnik kamilmielnik self-assigned this May 9, 2024
@kamilmielnik kamilmielnik mentioned this issue May 9, 2024
Merged
@kamilmielnik
Copy link
Contributor Author

kamilmielnik commented May 9, 2024 •
edited

I'm guessing this is a bug with the FE code that parses case from user input into an actual MBQL clause, it's probably not recursively calling the same parsing code on its arguments. offset needs to go thru code that adds the options map automatically

💯

Will be fixed with 5ddbe34 in #42326.
I'll close this issue for now as it is/was reproducible only in that PR.
Reopening, we should probably still merge it to master.

@kamilmielnik kamilmielnik reopened this May 10, 2024
kamilmielnik added a commit that referenced this issue May 10, 2024
@kamilmielnik
Add a repro for #42377
96c899a
@kamilmielnik kamilmielnik mentioned this issue May 10, 2024
Merged
kamilmielnik added a commit that referenced this issue May 10, 2024
@kamilmielnik
Fix - diagnose-expression throws when Offset is nested (#42497)
3e9f4ac 
* 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
@kamilmielnik kamilmielnik added this to the 0.50 milestone May 10, 2024
uladzimirdev pushed a commit that referenced this issue May 11, 2024
@kamilmielnik @uladzimirdev
Fix - diagnose-expression throws when Offset is nested (#42497)
509c0f5 
* 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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kamilmielnik kamilmielnik

Labels
.Frontend Querying/GUI Query builder catch-all, including simple mode .Team/QueryingComponents Type:Bug Product defects
Projects
None yet
Milestone
0.50
2 participants
@camsaul @kamilmielnik