Skip to content

fix: restore measure filters on comparison accessors (_delta, _delta_perc, _percent_of_total) - #9939

Merged
nishantmonu51 merged 1 commit into
mainfrom
nishant/fix-comparison-measure-filters
Sep 24, 2026
Merged

nishantmonu51 merged 1 commit into
mainfrom
nishant/fix-comparison-measure-filters

Conversation

@nishantmonu51

Copy link
Copy Markdown
Collaborator

Measure filters on comparison accessors were silently dropped by the unified expression filter refactor (#9746). Restores them.

  • The having post-processor puts the raw identifier from the URL (e.g. impressions_delta) into the subquery's measure list. JoinerFilterManager.parse then looked up a measure spec by that suffixed name, found nothing, and dropped the filter.
  • Strip the suffix with the existing stripMeasureSuffix helper so the manager is keyed on the base measure. MeasureFilterManager.reconcile already maps the having expression back to the right comparison type via mapExprToMeasureFilter, and commit rebuilds the suffix from the type, so no double suffix and the subquery measure list stays the base measure (same as 0.89).
  • Adds tests for _delta, _delta_perc and _percent_of_total: chip created for the base measure with the right type/value, condition applied once, URL param round trips unchanged.

Reproducing steps (on main):

  1. Open any explore dashboard with a time comparison enabled (e.g. AdBids, compare to previous period).
  2. Add a measure filter: Impressions → % change from → > 10, grouped by Publisher. The URL gets f.AdBids_metrics=publisher having (impressions_delta_perc GT 0.1).
  3. Reload the page, or paste that URL directly.
  4. Before: no measure chip is shown and the leaderboards are unfiltered. After: the Impressions chip shows % change > 10 and the filter is applied.

The same happens for impressions_delta gt 10 (absolute change) and impressions_percent_of_total gt 0.25 (percent of total). Plain impressions gt 10 was unaffected.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

Measure filters on `<measure>_delta`, `<measure>_delta_perc` and
`<measure>_percent_of_total` were dropped when parsing the filter param.
`JoinerFilterManager.parse` keyed the measure manager on the suffixed
identifier, which is not a measure in the metrics view, so the manager was
never created. Strip the suffix with the existing `stripMeasureSuffix`
helper so the chip is created for the base measure; `reconcile` already
reads the comparison type back from the having expression.
@nishantmonu51 nishantmonu51 added the blocker A release blocker issue that should be resolved before a new release label Sep 23, 2026
@nishantmonu51
nishantmonu51 merged commit 8d44276 into main Sep 24, 2026
16 checks passed
@nishantmonu51
nishantmonu51 deleted the nishant/fix-comparison-measure-filters branch September 24, 2026 05:48
nishantmonu51 added a commit that referenced this pull request Sep 24, 2026
Measure filters on `<measure>_delta`, `<measure>_delta_perc` and
`<measure>_percent_of_total` were dropped when parsing the filter param.
`JoinerFilterManager.parse` keyed the measure manager on the suffixed
identifier, which is not a measure in the metrics view, so the manager was
never created. Strip the suffix with the existing `stripMeasureSuffix`
helper so the chip is created for the base measure; `reconcile` already
reads the comparison type back from the having expression.

(cherry picked from commit 8d44276)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker A release blocker issue that should be resolved before a new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants