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

Lib.isNumber returns false for aggregation-based columns after join is removed #47184

Closed
Tracked by #46519
kamilmielnik opened this issue Aug 23, 2024 · 3 comments · Fixed by #47936
Closed
Tracked by #46519

Lib.isNumber returns false for aggregation-based columns after join is removed #47184

kamilmielnik opened this issue Aug 23, 2024 · 3 comments · Fixed by #47936
Assignees
@metamben
Labels
.Backend Priority:P2 Average run of the mill bug Querying/Parameters & Variables Filter widgets, field filters, variables etc. Reporting/Dashboards .Team/Querying Type:Bug Product defects
Milestone
0.50.26

Comments

@kamilmielnik
Copy link
Contributor

kamilmielnik commented Aug 23, 2024 •
edited
Loading

Describe the bug

2024-08-23.21-06-07.mp4

To Reproduce

this might not be the optimal path to reproduce and certainly isn't the only possible one

  1. New > Question > Orders > Save
  2. New > Model > Notebook > Use question from step (1) as data source > Join Reviews on Product ID = Product ID > Count > Save
  3. New Dashboard > Add new number filter
  4. Add model from step 2 to dashboard
  5. Try to connect the number filter to the model - notice that you can choose "Count" column
  6. Save dashboard
  7. Open model
  8. Edit query definition
  9. Remove join
  10. Save
  11. Go back to dashboard
  12. Try to connect the number filter to the model

Expected behavior

It should be possible to connect the "Count" column to the number filter

Information about your Metabase installation

master, 743ae09

Severity

P2

Additional info

This Lib.isNumber check gives unexpected result for the "Count" column:

metabase/frontend/src/metabase-lib/v1/parameters/utils/filters.ts

Line 52 in bcd1a90

return column => Lib.isNumber(column) && !Lib.isLocation(column);

@kamilmielnik kamilmielnik added Type:Bug Product defects Priority:P2 Average run of the mill bug .Needs Triage .Team/Querying Reporting/Dashboards Querying/Parameters & Variables Filter widgets, field filters, variables etc. and removed .Needs Triage labels Aug 23, 2024
@kamilmielnik kamilmielnik mentioned this issue Aug 26, 2024
Open
@kamilmielnik
Copy link
Contributor Author

kamilmielnik commented Aug 26, 2024 •
edited
Loading

TODO: When working on this issue, please search for https://github.com/metabase/metabase/issues/47184 in e2e/test/scenarios/dashboard-filters/dashboard-filters-query-stages.cy.spec.ts and uncomment related blocks.
These tests provide alternative reproduction paths (i.e. without the step to remove the join clause)

This suite will be added in #46958 and will go to an integration branch (#47167) before landing in master.

@metamben
Copy link
Contributor

metamben commented Sep 12, 2024 •
edited
Loading

@kamilmielnik, my repro fails already at step 5. Am I not following the steps properly?

This is because :effective-type is missing from the column. I've tried the easy fix of setting :effective-type to the value of :base-type if it's missing and that solved the problem. I still want to understand why it's missing in the first place (it's present in the card's metadata), so a bit more digging is needed.

@kamilmielnik
Copy link
Contributor Author

kamilmielnik commented Sep 12, 2024 •
edited
Loading

Am I not following the steps properly?

@metamben I think you are.
In my original video steps 3 and 4 are done in reverse order but that should not matter though.

It seems that something else must have gotten broken since this issue was reported.

metamben added a commit that referenced this issue Sep 13, 2024
@metamben
Ensure :effective-type for columns from an aggregation in a card
17d9acb 
Fixes #47184
@metamben metamben mentioned this issue Sep 13, 2024
Merged
1 task
metamben added a commit that referenced this issue Sep 16, 2024
@metamben
Ensure :effective-type for columns from an aggregation in a card (#47936
96d46e9 
)

* Ensure :effective-type for columns from an aggregation in a card

Fixes #47184

* Synchronize effective-type with the base-type on override

When :base-type in the column metadata is overridden with the :base-type in the field ref, set it as :effective-type
too.  If :effective-type is also set in the field ref, it wins.
metamben added a commit that referenced this issue Sep 16, 2024
@github-automation-metabase @metamben
Ensure :effective-type for columns from an aggregation in a card (#47936
e6f09d2 
) (#47942)

* Ensure :effective-type for columns from an aggregation in a card

Fixes #47184

* Synchronize effective-type with the base-type on override

When :base-type in the column metadata is overridden with the :base-type in the field ref, set it as :effective-type
too.  If :effective-type is also set in the field ref, it wins.

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>
@github-actions github-actions bot added this to the 0.50.26 milestone Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@metamben metamben

Labels
.Backend Priority:P2 Average run of the mill bug Querying/Parameters & Variables Filter widgets, field filters, variables etc. Reporting/Dashboards .Team/Querying Type:Bug Product defects
Projects
None yet
Milestone
0.50.26
Development

Successfully merging a pull request may close this issue.

Ensure :effective-type for columns from an aggregation in a card metabase/metabase
2 participants
@kamilmielnik @metamben