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

firing an x-ray made of aggregated fields will generate bad wrong queries #43793

Closed
paoliniluis opened this issue Jun 6, 2024 · 0 comments · Fixed by #44863
Closed

firing an x-ray made of aggregated fields will generate bad wrong queries #43793

paoliniluis opened this issue Jun 6, 2024 · 0 comments · Fixed by #44863
Assignees
@snoe
Labels
.Backend Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Processor .Team/Querying Type:Bug Product defects
Milestone
0.50.14

Comments

@paoliniluis
Copy link
Contributor

Describe the bug

If you try to x-ray some model that was made with aggregations, then the x-ray will fail with all questions being wrong

To Reproduce

  1. build a question like
    image
  2. save it as a model
  3. then do an x-ray on the model

see the dashboard failing on every question

Expected behavior

It should work

Logs

image

ERROR: column source.user_id does not exist
Position: 926

Information about your Metabase installation

v50, but probably comes from a previous version

Severity

P1

Additional context

NA
@paoliniluis paoliniluis added Type:Bug Product defects Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Processor Querying/X-rays Querying/Models aka Datasets labels Jun 6, 2024
@NevRA NevRA added .Backend .Team/QueryProcessor(deprecated) Use .Team/Querying instead and removed Querying/X-rays Querying/Models aka Datasets labels Jun 7, 2024
@snoe snoe self-assigned this Jun 20, 2024
snoe added a commit that referenced this issue Jun 28, 2024
@snoe
fix: limit xrays on agg models to source fields
0697d92 
Fixes #43793

1. `table-like?` was checking aggregations but not breakouts. This meant
   that legacy metric definitions would try to apply to the second stage
   which is generally illegal and would cause things like implicit joins
   to come in where they make no sense.
2. Trying to bin a `count` metric-like model would give a bin span of
   0.0 which would blow up those questions.
3. Trying to self reference breakout fields, in the precense of a
   question's  aggregation meant that most fields would not be exposed to
   the next stage and should be ignored. (similar to 1.)
@snoe snoe mentioned this issue Jun 28, 2024
Merged
@snoe snoe closed this as completed in e3378d4 Jul 5, 2024
snoe added a commit that referenced this issue Jul 5, 2024
@snoe
fix: limit xrays on agg models to source fields (#44863)
356f66c 
* fix: limit xrays on agg models to source fields

Fixes #43793

1. `table-like?` was checking aggregations but not breakouts. This meant
   that legacy metric definitions would try to apply to the second stage
   which is generally illegal and would cause things like implicit joins
   to come in where they make no sense.
2. Trying to bin a `count` metric-like model would give a bin span of
   0.0 which would blow up those questions.
3. Trying to self reference breakout fields, in the precense of a
   question's  aggregation meant that most fields would not be exposed to
   the next stage and should be ignored. (similar to 1.)

* Fix tests
snoe added a commit that referenced this issue Jul 15, 2024
@github-automation-metabase @snoe
🤖 backported "fix: limit xrays on agg models to source fields" (#45191)
ff6bd5f 
* fix: limit xrays on agg models to source fields

Fixes #43793

1. `table-like?` was checking aggregations but not breakouts. This meant
   that legacy metric definitions would try to apply to the second stage
   which is generally illegal and would cause things like implicit joins
   to come in where they make no sense.
2. Trying to bin a `count` metric-like model would give a bin span of
   0.0 which would blow up those questions.
3. Trying to self reference breakout fields, in the precense of a
   question's  aggregation meant that most fields would not be exposed to
   the next stage and should be ignored. (similar to 1.)

* Fix tests

Co-authored-by: Case Nelson <case@metabase.com>
@github-actions github-actions bot added this to the 0.50.14 milestone Jul 15, 2024
@perivamsi perivamsi added .Team/Querying and removed .Team/QueryProcessor(deprecated) Use .Team/Querying instead labels Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@snoe snoe

Labels
.Backend Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Processor .Team/Querying Type:Bug Product defects
Projects
None yet
Milestone
0.50.14
Development

Successfully merging a pull request may close this issue.

fix: limit xrays on agg models to source fields metabase/metabase
4 participants
@snoe @perivamsi @NevRA @paoliniluis