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.

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

GUI model joining native ones break with column not found error #40252

Closed
zbodi74 opened this issue Mar 18, 2024 · 8 comments · Fixed by #40933
Closed

GUI model joining native ones break with column not found error #40252

zbodi74 opened this issue Mar 18, 2024 · 8 comments · Fixed by #40933
Assignees
Labels
.Escalation Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Nested Queries Questions based on other saved questions Querying/ .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Type:Bug Product defects
Milestone

Comments

@zbodi74
Copy link

zbodi74 commented Mar 18, 2024

Describe the bug

After updating its metadata, GUI based model joining native models break with a column not found error.

To Reproduce

(See a simpler way to reproduce in the comments below).

  1. Create a native model wcount:
select year(created_at), count(*) as WidgetCount from products where category = 'Widget' group by year(created_at)
  1. Create a native model gcount:
select year(created_at), count(*) as GadgetCount from products where category = 'Gadget' group by year(created_at)
  1. Create a GUI model joined: inner join models created on step 1 and 2, on the year column. Save it.

  2. Edit the metadata of joined, and rename the count(*) fields: WidgetCount, GadgetCount. Save it.

  3. See it break:

image

Expected behavior

It should work.

Logs

n/a

Information about your Metabase installation

Seen this on master (stats) as of:

Built on 2024-03-18
Hash: 6710930

I could not reproduce this on v1.49.0, so I also added the .Regression label.

Severity

Probably P2 - I found it while troubleshooting something else), it might be a regression though.

Additional context

SQL generated:

SELECT
  "source"."EXTRACT(YEAR
FROM CREATED_AT)" AS "EXTRACT(YEAR FROM CREATED_AT)",
  "source"."GADGETCOUNT" AS "GADGETCOUNT",
  "wcount - EXTRACT(YEAR FROM CREATED_AT)"."EXTRACT(YEAR FROM CREATED_AT)" AS "wcount - EXTRACT(YEAR FROM CREATED_AT)__EXTRACT(YEA_8b131169",
  "wcount - EXTRACT(YEAR FROM CREATED_AT)"."WIDGETCOUNT" AS "wcount - EXTRACT(YEAR FROM CREATED_AT)__WIDGETCOUNT"
FROM
  (
    select
      year(created_at),
      count(*) as GadgetCount
    from
      products
    where
      category = 'Gadget'
    group by
      year(created_at)
  ) AS "source"
 
LEFT JOIN (
    select
      year(created_at),
      count(*) as WidgetCount
    from
      products
    where
      category = 'Widget'
    group by
      year(created_at)
  ) AS "wcount - EXTRACT(YEAR FROM CREATED_AT)" ON "source"."EXTRACT(YEAR FROM CREATED_AT)" = "wcount - EXTRACT(YEAR FROM CREATED_AT)"."EXTRACT(YEAR FROM CREATED_AT)"
LIMIT
  1048575
@zbodi74 zbodi74 added Type:Bug Product defects Querying/Nested Queries Questions based on other saved questions .Needs Triage .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Priority:P2 Average run of the mill bug Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness and removed Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Priority:P2 Average run of the mill bug labels Mar 18, 2024
@paoliniluis paoliniluis added Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness and removed .Needs Triage labels Mar 19, 2024
@zbodi74
Copy link
Author

zbodi74 commented Mar 21, 2024

This is now broken on 49.1. Here is a simpler reproduction:

  1. create native model A: select 1 as a1, 2 as a2
  2. create native model B: select 1 as b1, 2 as b2
  3. create GUI model A+B: A inner join B, on A.A1=B.B1
  4. edit the metadata of A+B: rename the third column from B - A1->B1 to anything else
    5.(!) save it and see the error:

Column "source.B1" not found; SQL statement:

SELECT "source"."A1" AS "A1",
       "source"."A2" AS "A2",
       "source"."B1" AS "B1",
       "source"."B2" AS "B2"
FROM (
    SELECT "source"."A1" AS "A1",
           "source"."A2" AS "A2",
            "B - A1"."B1" AS "B - A1__B1",
            "B - A1"."B2" AS "B - A1__B2"
    FROM (
        select 1 as a1,
               2 as a2 ) AS "source"
    LEFT JOIN (
        select 1 as b1, 
               2 as b2 ) AS "B - A1" 
    ON "source"."A1" = "B - A1"."B1"
) AS "source"
LIMIT 2000

@uladzimirdev
Copy link
Contributor

It's broken on 49.1 but works fine on 49.2

@zbodi74 could you please confirm?

@zbodi74
Copy link
Author

zbodi74 commented Mar 29, 2024

@uladzimirdev - confirmed, it works fine on 49.2.

@zbodi74 zbodi74 closed this as completed Mar 29, 2024
@uladzimirdev uladzimirdev reopened this Mar 29, 2024
@uladzimirdev
Copy link
Contributor

The issue still exists, the key point is inner join

@uladzimirdev
Copy link
Contributor

uladzimirdev commented Mar 29, 2024

I added a repro test for this issue #40772

also the test passed at CI and in simulating fast 3g, but fails when you run it locally on full speed (the issue comes from BE), maybe some race condition.

Stacktrace is added to the repro issue

@bshepherdson
Copy link
Contributor

bshepherdson commented Apr 1, 2024

For some reason this wouldn't repro for me on the first edit of the column name in the metadata. I edited it again and saw the issue.

The generated SQL shows the issue fairly plainly:

SELECT
  "source"."A1" AS "A1",
  "source"."A2" AS "A2",
  "source"."B1" AS "B1",
  "source"."B2" AS "B2"
FROM (
  SELECT
    "source"."A1" AS "A1",
    "source"."A2" AS "A2",
    "B - A1"."B1" AS "B - A1__B1",
    "B - A1"."B2" AS "B - A1__B2"
  FROM (select 1 as a1, 2 as a2) AS "source"
  INNER JOIN (select 1 as b1, 2 as b2) AS "B - A1"
  ON "source"."A1" = "B - A1"."B1"
) AS "source"
LIMIT 2000

The outermost layer's "source"."B1" references (source-alias) don't agree with the inner layer's AS "B - A1__B1" outputs (desired-alias). We're failing to match up the outer layer's incoming fields with the inner layer's output fields, so we lose track of the naming and default to just "B1", resulting in the error.

I've got a fix on the way for this.

@uladzimirdev
Copy link
Contributor

@bshepherdson have you tried running the linked repro e2e?

@bshepherdson
Copy link
Contributor

Not yet, but I'll be sure to test it as part of the PR.

bshepherdson added a commit that referenced this issue Apr 2, 2024
This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

Fixes #40252.
bshepherdson added a commit that referenced this issue Apr 2, 2024
This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

Fixes #40252.
@bshepherdson bshepherdson added this to the 0.49.4 milestone Apr 2, 2024
bshepherdson added a commit that referenced this issue Apr 3, 2024
This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

Fixes #40252.
bshepherdson added a commit that referenced this issue Apr 4, 2024
…0933)"

This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

See #40252.
bshepherdson added a commit that referenced this issue Apr 5, 2024
…0933)"

This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

See #40252.
bshepherdson added a commit that referenced this issue Apr 8, 2024
…0933)" (#40987)

This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

See #40252.

Co-authored-by: Braden Shepherdson <braden@metabase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Escalation Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Nested Queries Questions based on other saved questions Querying/ .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants