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

Fix column-less aliases resulting in phantom source columns #64

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

crisptrutski
Copy link
Collaborator

@crisptrutski crisptrutski commented Jun 18, 2024

Fixes metabase/metabase#44045

A bit of a mouthful.

The issue was that referring to a non-column alias could result in the Macaw interpreting those references as coming directly from one of the source tables. This was due to the fact that we ignoring aliases that were not introduced by named column references.

Clarifying example:

WITH cte AS (SELECT COUNT(*) AS a FROM foo)
SELECT a AS b FROM bar

Without this fix, we would have inferred {:table "bar" :column "a"} as a source column.

@crisptrutski crisptrutski changed the base branch from master to with-item-tables June 18, 2024 23:27
@crisptrutski crisptrutski self-assigned this Jun 18, 2024
@crisptrutski crisptrutski marked this pull request as draft June 18, 2024 23:28
@crisptrutski

This comment was marked as resolved.

@crisptrutski crisptrutski changed the title Test for references to column-less aliases not resulting in phantom source columns Fix column-less aliases not resulting in phantom source columns Jun 18, 2024
@crisptrutski crisptrutski marked this pull request as ready for review June 18, 2024 23:54
@crisptrutski

This comment was marked as resolved.

@crisptrutski crisptrutski changed the title Fix column-less aliases not resulting in phantom source columns Fix column-less aliases resulting in phantom source columns Jun 19, 2024
Copy link
Collaborator Author

crisptrutski commented Jun 21, 2024

Merge activity

  • Jun 21, 7:49 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
  • Jun 21, 7:57 AM EDT: @crisptrutski merged this pull request with Graphite.

@crisptrutski crisptrutski merged commit ba85f05 into master Jun 21, 2024
4 checks passed
@crisptrutski crisptrutski deleted the test-constant-references branch June 21, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't expose elements depending on column-less expressions
2 participants