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.

Sign up for GitHub

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

Jump to bottom

Merge metadata correctly #36153

Closed
Tracked by #36015
markbastian opened this issue Nov 27, 2023 · 1 comment · Fixed by #36245
Closed
Tracked by #36015

Merge metadata correctly #36153

markbastian opened this issue Nov 27, 2023 · 1 comment · Fixed by #36245
Assignees
@markbastian @adam-james-v
Labels
no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team
Milestone
0.49

Comments

@markbastian
Copy link
Contributor

markbastian commented Nov 27, 2023 •
edited
Loading

Currently, the results_metadata from executing the query are used (see below) and user-defined metadata is ignored. The result of this effort should be a unified path, preferably something like a middleware in metabase.query-processor.middleware.results-metadata that correctly unifies all of the result metadata in one location.

  • Currently, the results_metadata from executing the query are used
  • We need to ensure that existing result_metadata from cards (e.g. user curated model metadata) is retained and preferred to the computed metadata
  • A likely place to put this logic is in metabase.query-processor.middleware.results-metadata, but we need to be wary of record-and-return-metadata! as it can smash existing metadata. However, if we fixed it here, it would do the right thing everywhere.
  • An alternative location is in the rendering logic, but this isn't preferred as it's close to the leaves of the logic.
  • AC
    • Unit tests demonstrating the precedence of user-defined results_metadata over generated result_metadata in multiple card renders:
      • render/render-pulse-card :inline and :table as viz type, minimally
      • Note: Anything inside an <svg> tag comes from the js side of rendering. Anything happening in <html> happens on the Clojure side.
This was referenced Nov 27, 2023
Closed
Closed
@markbastian markbastian added no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team labels Nov 27, 2023
@markbastian markbastian mentioned this issue Nov 30, 2023
Merged
@markbastian
Copy link
Contributor Author

There is actually an existing way to add in the metadata from a model but it isn't obvious from our dev.render-png ns. The inbound PR is very clear as to what cases need special handling (basically when you have a model with custom metadata) and how to use it.

@markbastian markbastian modified the milestones: 0.48.3, 0.49 Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@markbastian markbastian

@adam-james-v adam-james-v

Labels
no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team
Projects
None yet
Milestone
0.49
Development

Successfully merging a pull request may close this issue.

Document and test adding results_metadata to qp middleware for static viz metabase/metabase
2 participants
@markbastian @adam-james-v