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

[MLv2] Drop redundant :fields clauses from stages and joins #36959

Merged
merged 1 commit into from Jan 2, 2024

Conversation

bshepherdson
Copy link
Contributor

If they're equivalent to the default, they can be removed (for stages)
or replaced by :all (on joins).

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@metabase-bot metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label Dec 19, 2023
@bshepherdson bshepherdson requested review from metamben and a team December 19, 2023 22:20
@bshepherdson bshepherdson added the no-backport Do not backport this PR to any branch label Dec 19, 2023
@bshepherdson bshepherdson added this to the 0.49 milestone Dec 19, 2023
Copy link

cypress bot commented Dec 19, 2023

Passing run #1285 ↗︎

0 2201 155 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

[MLv2] Drop redundant :fields clauses from stages and joins
Project: Metabase e2e Commit: 5ed14d9b88
Status: Passed Duration: 16:24 💡
Started: Jan 2, 2024 9:02 PM Ended: Jan 2, 2024 9:18 PM

Review all test suite changes for PR #36959 ↗︎

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, I've minor comments only.

src/metabase/lib/equality.cljc Outdated Show resolved Hide resolved
src/metabase/lib/field.cljc Outdated Show resolved Hide resolved
src/metabase/lib/remove_replace.cljc Show resolved Hide resolved
export function findColumnIndexForColumnSetting(columns, columnSetting) {
// NOTE: need to normalize field refs because they may be old style [fk->, 1, 2]
const fieldRef = normalizeFieldRef(columnSetting.fieldRef);
// first try to find by fieldRef
if (fieldRef != null) {
const index = _.findIndex(columns, col =>
_.isEqual(fieldRef, normalizeFieldRef(fieldRefForColumn(col))),
areFieldRefsEqual(fieldRef, normalizeFieldRef(fieldRefForColumn(col))),
Copy link
Contributor

@ranquild ranquild Jan 2, 2024

Choose a reason for hiding this comment

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

Dimension.parseMBQL(fieldRef).isSameBaseDimenson(normalizeFieldRef(fieldRefForColumn(col))) should do the same thing

Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

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

Please use Dimension.parseMBQL(fieldRef).isSameBaseDimenson(). Approving to not block the PR

If they're equivalent to the default, they can be removed (for stages)
or replaced by `:all` (on joins).
@bshepherdson bshepherdson force-pushed the mblib-drop-redundant-fields-clauses branch from 5a9a0c4 to fb6b3b4 Compare January 2, 2024 20:49
@bshepherdson bshepherdson merged commit c382f56 into master Jan 2, 2024
105 checks passed
@bshepherdson bshepherdson deleted the mblib-drop-redundant-fields-clauses branch January 2, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryProcessor :hammer_and_wrench:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants