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

Query Builder shouldn't use field-literal to refer to Fields from MBQL nested queries #14824

Closed
camsaul opened this issue Feb 15, 2021 · 1 comment
Closed

Query Builder shouldn't use field-literal to refer to Fields from MBQL nested queries #14824

camsaul opened this issue Feb 15, 2021 · 1 comment
Labels
.Frontend Querying/GUI Query builder catch-all, including simple mode Type:Tech Debt or Refactoring

Comments

@camsaul
Copy link
Member

camsaul commented Feb 15, 2021

I noticed this when I was working on #14812 -- see comment I left here: https://github.com/metabase/metabase/pull/14812/files/06202be4f8f5c02b0b0609a0017606bed0c790ef#diff-1536103ee504ae4476ddaa2b633dff7d0e31a56647ba11b383c98962f757d993R309

With the 0.38.0 changes, the only time we should use field-literal is to refer to columns coming back from native queries. We should use field-id everywhere else. Either way, in 0.38.0+, the results metadata comes back with field_ref, so we should use that instead. There's no need to second-guess things in the FE client. field_ref will always be the preferred way to refer to a Field. We can work around it for now on the backend, but going forward it's probably better to do it the right way.

I tried making this change to the FE but it broke other stuff since it seems the id here is used to populate dropdowns (the name is fetched from the field-literal in id rather than from field().name or field().display_name from the metadata)
@camsaul camsaul mentioned this issue Feb 15, 2021
Merged
kulyk added a commit that referenced this issue Sep 3, 2021
@kulyk
Don't use MBQL field literals in notebook tests
d09a8f8 
#14824
kulyk added a commit that referenced this issue Sep 14, 2021
@kulyk
Fix removing a notebook section can remove other sections (#17727)
277e83f 
* Add test ID for notebook's action buttons

* Add repro for #17712

* Use `Join.clean` in join step's clean callback

* Always show a remove icon

* Fix referencing an empty dimension

* Improve Join's validity check

* Don't cleanup incomplete join sections

* Fix structured query tests

* Don't use MBQL field literals in notebook tests

#14824
@kulyk kulyk mentioned this issue Oct 28, 2021
Merged
@flamber flamber added Querying/GUI Query builder catch-all, including simple mode and removed Query Builder labels Nov 15, 2021
@camsaul
Copy link
Member Author

camsaul commented Oct 2, 2023

Fixed by #28689

@camsaul camsaul closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
.Frontend Querying/GUI Query builder catch-all, including simple mode Type:Tech Debt or Refactoring
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@flamber @camsaul