search

metabase / metabase

The simplest, fastest way to get business intelligence and analytics to everyone in your company :yum:
https://metabase.com
Other
38.56k stars 5.11k forks source link

`lib.aggregation/aggregation-column` can try to `find-matching-column` on an expression, rather than the inner ref #48567

Open bshepherdson opened 2 weeks ago

bshepherdson commented 2 weeks ago

Describe the bug

metabase.models.field-usage/aggregation->field-usage wants to find all the field refs inside an aggregation.

lib.aggregation/aggregation-column naively assumes the 2nd element of an aggregation clause must be a direct ref. That's not true - it can be a subexpression that contains field refs for eg. a :sum-where.

  1. That's straight-up a bug in aggregation-column and should be fixed. (By picking the inner column correctly for each kind of aggregation.)
  2. But it's also a problem for aggregation->field-usage which actually wants both the field being eg. summed and any fields referenced in the :sum-where condition!

So some deeper change is needed to support aggregation->field-usage; probably a new lib.aggregation/aggregation-filter that returns the filtering expression for :count-where and :sum-where. (models.field-usage has logic for pulling apart expressions already.)

To Reproduce

  1. Write a :sum-where aggregation.
  2. Run the query
  3. Observe the "Unknown type of ref" error on the client where find-match-column was called with the filter expression as the ref, rather than the column getting summed.

Expected behavior

Correctly finding field usage in all cases!

Logs

No response

Information about your Metabase installation

any version; eg. master

Severity

Log spam, also a gap in field usage stats

Additional context

No response

appleby commented 2 weeks ago

There is a similar bug in filter->field-usage as well where filter-parts also does not handle nested filter expressions

https://github.com/metabase/metabase/issues/44376#issuecomment-2394420271