Don't offer drill-throughs when top-level column doesn't exist #66907
+140
−41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
e2e tests failed on
|
| File | Test Name |
|---|---|
dashboardsAndQuestions.cy.spec.ts |
Cache invalidation for dashboards and questions > ee > adaptive and duration strategies > questions can use duration strategy |
embed-flow-enable-embed-js.cy.spec.ts |
scenarios > embedding > sdk iframe embed setup > enable embed js > ee > shows the Enable to Continue button and enables embedding on click |
embed-options.cy.spec.ts |
scenarios > embedding > sdk iframe embed options passthrough > shows a static question with drills=false |
Comment on lines
144
to
+145
| pivots (into {} (for [pivot-type breakout-pivot-types | ||
| :let [pred (get pivot-type-predicates pivot-type) | ||
| columns (pivot-drill-pred query stage-number context pred)] | ||
| :when (not-empty columns)] | ||
| :let [pred (get pivot-type-predicates pivot-type) |
Contributor
There was a problem hiding this comment.
The :let and :when keywords in the for comprehension should be aligned consistently. According to the Metabase Clojure style, when using for bindings, the binding clauses should be vertically aligned.
Consider:
Suggested change
| pivots (into {} (for [pivot-type breakout-pivot-types | |
| :let [pred (get pivot-type-predicates pivot-type) | |
| columns (pivot-drill-pred query stage-number context pred)] | |
| :when (not-empty columns)] | |
| :let [pred (get pivot-type-predicates pivot-type) | |
| pivots (into {} (for [pivot-type breakout-pivot-types | |
| :let [pred (get pivot-type-predicates pivot-type) | |
| columns (pivot-drill-pred query stage-number context pred)] | |
| :when (not-empty columns)] |
The extra spaces after :let and :when are inconsistent with standard formatting where keywords should have consistent spacing.
github-automation-metabase
pushed a commit
that referenced
this pull request
Dec 15, 2025
The summary: we are offering drill-throughs on custom columns defined after aggregation, but those custom columns don't exist in the base query. The error presented in such a way that `lib.underlying/top-level-column` for a dimension was returning `nil`, and we were happily putting that `nil` into the query we constructed. I created a utility to filter dimensions by whether or not the top-level-column can be resolved, and used this for the underlying-records drill-through (from the original issue) as well as the pivot drill-through (which behaved similarly). If no dimensions have a top-level column, the drill-through is not available. If some do, we offer the partial drill-through.
Contributor
Code Review SummaryI've reviewed the Clojure code changes in this PR. The implementation looks solid and addresses issue #66715 correctly by filtering out dimensions that can't be traced back to the top-level query. Style Issues Found:
Implementation Notes:✅ The new traceable-dimensions function is well-designed and properly documented The solution appropriately addresses the root cause by preventing drill-throughs when dimensions reference columns that don't exist in the top-level query, rather than allowing them and showing an error. |
github-automation-metabase
pushed a commit
that referenced
this pull request
Dec 15, 2025
The summary: we are offering drill-throughs on custom columns defined after aggregation, but those custom columns don't exist in the base query. The error presented in such a way that `lib.underlying/top-level-column` for a dimension was returning `nil`, and we were happily putting that `nil` into the query we constructed. I created a utility to filter dimensions by whether or not the top-level-column can be resolved, and used this for the underlying-records drill-through (from the original issue) as well as the pivot drill-through (which behaved similarly). If no dimensions have a top-level column, the drill-through is not available. If some do, we offer the partial drill-through.
github-automation-metabase
added a commit
that referenced
this pull request
Dec 16, 2025
… (#66986) The summary: we are offering drill-throughs on custom columns defined after aggregation, but those custom columns don't exist in the base query. The error presented in such a way that `lib.underlying/top-level-column` for a dimension was returning `nil`, and we were happily putting that `nil` into the query we constructed. I created a utility to filter dimensions by whether or not the top-level-column can be resolved, and used this for the underlying-records drill-through (from the original issue) as well as the pivot drill-through (which behaved similarly). If no dimensions have a top-level column, the drill-through is not available. If some do, we offer the partial drill-through. Co-authored-by: Timothy S. Dean <timothy.dean@metabase.com>
github-automation-metabase
added a commit
that referenced
this pull request
Dec 16, 2025
… (#66987) The summary: we are offering drill-throughs on custom columns defined after aggregation, but those custom columns don't exist in the base query. The error presented in such a way that `lib.underlying/top-level-column` for a dimension was returning `nil`, and we were happily putting that `nil` into the query we constructed. I created a utility to filter dimensions by whether or not the top-level-column can be resolved, and used this for the underlying-records drill-through (from the original issue) as well as the pivot drill-through (which behaved similarly). If no dimensions have a top-level column, the drill-through is not available. If some do, we offer the partial drill-through. Co-authored-by: Timothy S. Dean <timothy.dean@metabase.com>
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #66715
Description
The issue is well-documented. The summary: we are offering drill-throughs on custom columns defined after aggregation, but those custom columns don't exist in the base query.
I see three options:
Option 3 was suggested by @alexyarosh on #66715, and it's the one I went with here. The error presented in such a way that
lib.underlying/top-level-columnfor a dimension was returningnil, and we were happily putting thatnilinto the query we constructed.I created a utility to filter dimensions by whether or not the top-level-column can be resolved, and used this for the underlying-records drill-through (from the original issue) as well as the pivot drill-through (which behaved similarly). If no dimensions have a top-level column, the drill-through is not available. If some do, we offer the partial drill-through.
How to verify
Copied from the issue:
For an artisanal handcrafted version:
Demo
Loom
Checklist