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 subdimensions for a join table #15506

Closed
wants to merge 1 commit into from
Closed

Conversation

ariya
Copy link
Contributor

@ariya ariya commented Apr 6, 2021

This fixes #15446. To verify, run the Cypress test:

yarn test-cypress-no-build --spec frontend/test/metabase/scenarios/question/notebook.cy.spec.js

Steps to reproduce:

  1. Ask a question, Custom question.
  2. Sample Dataset, Orders table.
  3. Join Data, People table.
  4. Summarize, "Sum of Total"
  5. Group by: "People - Users" and then "Created At".
  6. Hover and then click on the "by month >"
  7. The pop-up should show a list of binning choices.

Before:

There are duplicated entries in the pop-up. Note that the pop-up is too small (vertically), scroll around to check that in fact some entries (e.g. "Minute") appear twice.

image

After:

There is no more duplication in the pop-up.

image

@ariya
Copy link
Contributor Author

ariya commented Apr 6, 2021

Note that the Dimension logic has changed dramatically since PR #14897 (MBQL Refactor), i.e. there is no more specialized class such as JoinedDimension. Hopefully this PR does a sensible re-implementation of the logic.

Copy link
Contributor

@paulrosenzweig paulrosenzweig left a comment

Choose a reason for hiding this comment

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

I don't understand exactly where the now filtered out dimensions were originating. I guess dimension_options? It seems like we might be able to construct the right list rather than filtering down at the end. That's a vague hypothesis though. I think shipping this is good!

@ariya
Copy link
Contributor Author

ariya commented Apr 7, 2021

I don't understand exactly where the now filtered out dimensions were originating. I guess dimension_options? It seems like we might be able to construct the right list rather than filtering down at the end. That's a vague hypothesis though. I think shipping this is good!

It is originated from the usual _dimensionForOption() function. But I think you raise a very good point here: I definitely should investigate the different of behavior of the said function, before and after MBQL field refactoring. Perhaps the underlying issue is the construction of the initial subdimensions (when taking into account a joined table).

@ariya ariya marked this pull request as draft April 7, 2021 17:06
@ariya
Copy link
Contributor Author

ariya commented Apr 8, 2021

Closing this, in favor of the updated approach in PR #15530.

@ariya ariya closed this Apr 8, 2021
@ariya ariya deleted the subdimensions-join branch July 13, 2021 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants