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

[QP] Fix confusion of expressions with the same name as columns #39255

Merged
merged 1 commit into from Mar 4, 2024

Conversation

bshepherdson
Copy link
Contributor

When determining column aliases in add-alias-info, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.

Copy link
Contributor Author

bshepherdson commented Feb 28, 2024

@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Feb 28, 2024
@bshepherdson bshepherdson marked this pull request as ready for review February 28, 2024 21:55
@bshepherdson bshepherdson added this to the 0.49 milestone Feb 28, 2024
Copy link

replay-io bot commented Feb 28, 2024

Status Complete ↗︎
Commit 3fe16d5
Results
⚠️ 13 Flaky
2324 Passed

@bshepherdson bshepherdson force-pushed the qp-handle-same-name-expressions branch 2 times, most recently from 70c7c4d to 30ccd18 Compare March 1, 2024 13:32
@bshepherdson bshepherdson added backport Automatically create PR on current release branch on merge and removed .Team/QueryProcessor :hammer_and_wrench: labels Mar 1, 2024
When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.
@bshepherdson bshepherdson force-pushed the qp-handle-same-name-expressions branch from 30ccd18 to 3fe16d5 Compare March 1, 2024 15:47
@bshepherdson bshepherdson enabled auto-merge (squash) March 4, 2024 14:29
Copy link
Contributor

@metamben metamben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +324 to +327
(defn- field-requires-original-field-name
"JSON extraction fields need to be named with their outer `field-name`, not use any existing `::desired-alias`."
[field-clause]
(boolean (some-> field-clause field-instance :nfc-path)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test for this situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but unfortunately they're DB-specific because JSON unfolding support is not widespread.

@bshepherdson bshepherdson merged commit 0506255 into master Mar 4, 2024
109 checks passed
@bshepherdson bshepherdson deleted the qp-handle-same-name-expressions branch March 4, 2024 14:38
bshepherdson added a commit that referenced this pull request Mar 4, 2024
When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.
bshepherdson added a commit that referenced this pull request Mar 4, 2024
When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.
bshepherdson added a commit that referenced this pull request Mar 4, 2024
bshepherdson added a commit that referenced this pull request Mar 4, 2024
…) (#39526)

When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.

Co-authored-by: Braden Shepherdson <braden@metabase.com>
bshepherdson added a commit that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge
Projects
None yet
2 participants