fix(Filters): Apply native & cross filters on common columns

[geido]

SUMMARY

When in scope (global or specific) charts that are not applicable to the filter (do not share the same datasource or columns) were being applied anyway, causing unnecessary queries. This PR checks that the charts have columns in common or will exclude the charts.

AFTER

COVID.Vaccine.Dashboard.mp4

COVID.Vaccine.Dashboard.1.mp4

TESTING INSTRUCTIONS

1. Create a native filter that has no common columns with a chart
2. Add the chart in scope
3. Apply the filter
4. The chart should not reload and emit a query

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[kgabryje]

/testenv up

[github-actions]

@kgabryje Ephemeral environment spinning up at http://35.90.248.247:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[geido]

Just realized that highlighting a filter that is not applicable should also be added as an enhancement for this PR. Working on that.

[sadpandajoe]

/testenv up

[github-actions]

@sadpandajoe Ephemeral environment spinning up at http://35.88.232.227:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[sadpandajoe]

Just a quick question, should it affect all charts if it's from the same database table even if one is virtual? Example here: http://35.88.232.227:8080/superset/dashboard/11/?native_filters_key=vORa9YtGn5rLPgi2AjgirnIJB_30m_cpUE3kzlqWFgMGjPIw3CKoFrTKjNNAUkan

1. go to sql lab
2. run the query

SELECT * 
FROM covid_vaccines 
WHERE country_name = "United States"

3. save as a new dataset
4. copy one of the charts that use covid_vaccines
5. switch the dataset to the copied one
6. put this on the covid vaccines dashboard
7. do a cross filter on the newly created dataset chart

right now following this step, all charts are affected by the virtual dataset.

[sadpandajoe]

Spoke with @yousoph and it sounds like my case is invalid so going to approve.

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[Mainer-g00t]

All our dashboards utilize cross-filters across various virtual datasets, and we use Jinja to capture and apply filters, even if the filtered columns in the filter are not present in the dataset.
For example:

• dataset & chart 1: city, number of habitants
• dataset & chart 2: profession, number of proffesionals
  We filter between both using the filter in the WHERE statement. The reason for having different datasets is that the cardinality in our tables is very high and a single dataset ruins performance.

I'm afraid that if this is applied automatically to every case could basically break all of our dashboards

[fmannhardt]

Not sure if this should go here or to a new issue (and I won't manage to find time to create a reproducible example in the next week). However, I re-tested this feature together with cross filters and the following setup:

• A renamed column (colum_a AS "Colum A") is used as source for the cross filter from chart A (with data source A)
• A second chart B from a different data source B has the same underlying column (column_a) but this column is only used in a filter of that chart that is based on an JINJA expression (using get_filters() as explained here: https://superset.apache.org/docs/configuration/sql-templating/)
• Result is that the cross filter on chart A does not affect chart B.

I looked through the code and it, in fact, does not consider searching for JINJA in the filter of other charts:

function checkForExpression(formData?: Record<string, any>) {
  const groupby = ensureIsArray(formData?.groupby) ?? [];
  const allColumns = ensureIsArray(formData?.all_columns) ?? [];
  const checkColumns = groupby.concat(allColumns);
  return checkColumns.some(
    (col: string | Record<string, any>) =>
      typeof col !== 'string' && col.expressionType !== undefined,
  );
}

Shouldn't this also look through the adhoc_filters?

There is a workaround to define a calculated column which matches the label of the emitter column. So, adding a calculated column "Colum A", which is simply using colum_a as SQL expression, to dataset B helps. But the whole approach to cross filter based on labels seems very brittle.

[villebro]

I didn't review these changes, but it was my understanding that

1. by default, native and cross filters will only be scoped to charts using the same dataset
2. If one wants the filters to apply to charts that aren't using the same dataset, one simply adds them to the filter scope

Similar to the comments above, I know of many use cases where there's jinja logic that intercepts filters for columns that aren't present in the dataset, and use those to modify the chart query somehow. I'm not sure if this has been explicitly stated as a feature that Superset supports, but I think this use case has been well established for a longer time already, hence I feel removing it will be problematic.

Does this PR break said workflow? I'm happy to jump on a sync call to discuss this if needed.

[michael-s-molina]

1. by default, native and cross filters will only be scoped to charts using the same dataset

@geido was really careful to not break the compatibility. For that reason, columns with the same name are also affected even if they belong to different datasets. This was some legacy behavior that @geido rightfully maintained.

2. If one wants the filters to apply to charts that aren't using the same dataset, one simply adds them to the filter scope

Looking at the video below, where each chart is from a different dataset, we can see that if I modify the scope, the chart is reloaded according to the scope. I noticed that the query is not being modified when reloading the chart but I don't know if this was intentional or if we found a bug. @geido

Screen.Recording.2024-10-21.at.16.16.14.mov

[villebro]

@Mainer-g00t @fmannhardt as you raised concerns here, did you find that this PR broke your use cases?

[Mainer-g00t]

@Mainer-g00t @fmannhardt as you raised concerns here, did you find that this PR broke your use cases?

Yes, it does. In our case, charts export filters that are picked up by jinja logic to filter dataset with data that is not part of the columns in the dataset. Specifically, our dataset looks like:

• customer (1k), products (1k), providers(1k), metrics...

This leads to a roughly 500M row table. We split it in 3 datasets, one for each dimension and use x-filters to select on the other datasets. ie, clicking on one customer on the first table will filter all products and providers for that customer and so on.

This allows us to display relationships in big datasets breaking it up in dimensions and using x-filters

[villebro]

@Mainer-g00t @fmannhardt as you raised concerns here, did you find that this PR broke your use cases?

Yes, it does. In our case, charts export filters that are picked up by jinja logic to filter dataset with data that is not part of the columns in the dataset. Specifically, our dataset looks like:

• customer (1k), products (1k), providers(1k), metrics...

This leads to a roughly 500M row table. We split it in 3 datasets, one for each dimension and use x-filters to select on the other datasets. ie, clicking on one customer on the first table will filter all products and providers for that customer and so on.

This allows us to display relationships in big datasets breaking it up in dimensions and using x-filters

Just to confirm, have you tested the changes here and can confirm that they do indeed break your use case? Because to me it seems like you should still be able to do the same cross filtering even after this change, as long as you've defined your filter scopes correctly.

[Mainer-g00t]

@Mainer-g00t @fmannhardt as you raised concerns here, did you find that this PR broke your use cases?

Yes, it does. In our case, charts export filters that are picked up by jinja logic to filter dataset with data that is not part of the columns in the dataset. Specifically, our dataset looks like:

• customer (1k), products (1k), providers(1k), metrics...

This leads to a roughly 500M row table. We split it in 3 datasets, one for each dimension and use x-filters to select on the other datasets. ie, clicking on one customer on the first table will filter all products and providers for that customer and so on.
This allows us to display relationships in big datasets breaking it up in dimensions and using x-filters

Just to confirm, have you tested the changes here and can confirm that they do indeed break your use case? Because to me it seems like you should still be able to do the same cross filtering even after this change, as long as you've defined your filter scopes correctly.

Yes, we just tested it on 4.1 and breaks cross filtering for us. It does not apply filters if the column is not in the dataset even when it is used in the WHERE statement with jinja

[geido]

Thanks @Mainer-g00t and @fmannhardt. Let me have a deeper look at what you are mentioning.

[fmannhardt]

Thanks. Just to confirm it does break our use case in 4.1rc3 for two underlying reasons: (1) The column filtering is renamed with a label (colx as X). So, cross filtering does consider it as X. (2) We used JINJA to create a custom filter condition based on selected filters (on X) but since there there is no column X in the dataset, it won't do the filtering on the chart. Btw, it does filter correctly when clicking on show the data as table or as SQL statement. So, the filtering is inconsistent. I think the code checking for expressions (quoted above) simply does not check there WHERE filters defined on the charts. It may also have issues with custom charts.

…

________________________________ From: Geido ***@***.***> Sent: Tuesday, October 22, 2024 2:57:19 PM To: apache/superset ***@***.***> Cc: Felix ***@***.***>; Mention ***@***.***> Subject: Re: [apache/superset] fix(Filters): Apply native & cross filters on common columns (PR #30438) Thanks @Mainer-g00t<https://github.com/Mainer-g00t> and @fmannhardt<https://github.com/fmannhardt>. Let me have a deeper look at what you are mentioning. — Reply to this email directly, view it on GitHub<#30438 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAYN7NVWRPCG2GXD4CF4KJDZ4ZDS7AVCNFSM6AAAAABPDK75DSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRZGIYTINBSHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[michael-s-molina]

Hi @fmannhardt @Mainer-g00t @geido. May I suggest opening a specific GitHub issue for this so we can track it here?

[Mainer-g00t]

Hi @fmannhardt @Mainer-g00t @geido. May I suggest opening a specific GitHub issue for this so we can track it here?

Hello, sure, I'll try to do it today with an example of the behaviour before and after the change

[fbgeobit]

Hi @michael-s-molina ,
We opened a new issue with use case commented by @Mainer-g00t : #30687

Thanks

[michael-s-molina]

Thank you! I added the issue to the 4.1 board.