fix(template_processing): get_filters now works for IS_NULL and IS_NOT_NULL operators

[Prokos]

SUMMARY

The macro get_filters within the Jinja context can be used to implement filtering in custom locations in your virtual dataset SQL. This works great, except for when filtering with the IS_NULL or IS_NOT_NULL operator. These are ignored, cannot be used, and will break certain implementations that rely on remove_filter to be functional (like the one I've outlined below).

The reason the code breaks for these operators is because they are unique in the sense that they don't have a comparator. Adjusting the code to not need a comparator for these operator types fixes the issue.

TESTING INSTRUCTIONS

1. Create a virtual dataset on BigQuery with the following SQL

• likely reproducible with less - but it's vital that both column selection/grouping and filtering is handled
• not a BigQuery specific bug, thats just the database I use

{% set finalColumns = columns if columns is defined else ['__ALL__'] %}

SELECT


  sum(measure) as measure,
  
  {% for col in finalColumns %}

  	{% if col == 'col1' or col == '__ALL__' or (col is mapping and 'sqlExpression' in col and col.sqlExpression == 'col1') %}
  		col1
  		{%- if col == '__ALL__' -%},{%- endif -%}
  		{%- if not loop.last -%},{%- endif -%}
  	{% endif %}
  	
  	{% if col == 'col2' or col == '__ALL__' or (col is mapping and 'sqlExpression' in col and col.sqlExpression == 'col2') %}
  		col2
  		{%- if col == '__ALL__' -%},{%- endif -%}
  		{%- if not loop.last -%},{%- endif -%}
  	{% endif %}
  	
  {% endfor %}
  
FROM UNNEST([
  STRUCT(1 as measure, 'value1A' as col1, 'value2A' as col2),
  STRUCT(2 as measure, 'value1A' as col1, 'value2B' as col2),
  STRUCT(5 as measure, 'value1B' as col1, 'value2B' as col2),
  STRUCT(15 as measure, NULL as col1, NULL as col2)
])

-- FILTERS

{% for filter in get_filters('col1', remove_filter=True) -%}
    {%- if loop.first %} WHERE 1 = 1 {% endif %}
    AND
    {% if filter.get('op') == 'IN' -%}
        col1 IN {{ filter.get('val')|where_in }}
    {% elif filter.get('op') == 'NOT IN' -%}
        col1 NOT IN {{ filter.get('val')|where_in }}
    {% elif filter.get('op') == 'ILIKE' -%}
        LOWER(col1) LIKE LOWER({{ filter.get('val') }})
    {% elif filter.get('op') == 'IS NULL' -%}
        col1 IS NULL
    {% elif filter.get('op') == 'IS NOT NULL' -%}
        col1 IS NOT NULL
    {% else -%}
        col1 {{ filter.get('op') }} {{ filter.get('val') }}
    {%- endif -%}
{%- endfor %}

{% for filter in get_filters('col2', remove_filter=True) -%}
    {% if loop.first %} WHERE 1 = 1 {% endif %}
    AND
    {% if filter.get('op') == 'IN' -%}
        col2 IN {{ filter.get('val')|where_in }}
    {% elif filter.get('op') == 'NOT IN' -%}
        col2 NOT IN {{ filter.get('val')|where_in }}
    {% elif filter.get('op') == 'ILIKE' -%}
        LOWER(col2) LIKE LOWER({{ filter.get('val') }})
    {% elif filter.get('op') == 'IS NULL' -%}
        col2 IS NULL
    {% elif filter.get('op') == 'IS NOT NULL' -%}
        col2 IS NOT NULL
    {% else -%}
        col2 {{ filter.get('op') }} {{ filter.get('val') }}
    {%- endif -%}
{%- endfor %}

-- GROUP BY
  
{% set ns = namespace(groupByEmitted=false) %}

{% for col in finalColumns %}

	{% if col == 'col1' or col == '__ALL__' or (col is mapping and 'sqlExpression' in col and col.sqlExpression == 'col1') %}
		{%- if not ns.groupByEmitted %}
			GROUP BY
			{% set ns.groupByEmitted = true %}
		{% endif -%}
		col1
		{%- if col == '__ALL__' -%},{%- endif -%}
		{%- if not loop.last -%},{%- endif -%}
	{% endif %}
	
	{% if col == 'col2' or col == '__ALL__' or (col is mapping and 'sqlExpression' in col and col.sqlExpression == 'col2') %}
		{%- if not ns.groupByEmitted %}
			GROUP BY
			{% set ns.groupByEmitted = true %}
		{% endif -%}
		col2
		{%- if not loop.last -%},{%- endif -%}
	{% endif %}

{% endfor %}
  

3. Click "Create Chart"
4. Set a filter on one of the columns but don't select it as a dimension:

5. On master, this breaks due to the following resulting SQL:

SELECT `measure` AS `measure`, `col2` AS `col2` 
FROM (
  SELECT
    sum(measure) as measure,
    col2
    
  FROM UNNEST([
    STRUCT(1 as measure, 'value1A' as col1, 'value2A' as col2),
    STRUCT(2 as measure, 'value1A' as col1, 'value2B' as col2),
    STRUCT(5 as measure, 'value1B' as col1, 'value2B' as col2),
    STRUCT(15 as measure, NULL as col1, NULL as col2)
  ])
	
  GROUP BY		
		col2
) AS `virtual_table` 
WHERE `col1` IS NULL
 LIMIT 1000

7. With this fix implemented, this works and the resulting SQL looks like this:

SELECT `measure` AS `measure`, `col2` AS `col2` 
FROM (

SELECT

  sum(measure) as measure,
  col2
  
FROM UNNEST([
  STRUCT(1 as measure, 'value1A' as col1, 'value2A' as col2),
  STRUCT(2 as measure, 'value1A' as col1, 'value2B' as col2),
  STRUCT(5 as measure, 'value1B' as col1, 'value2B' as col2),
  STRUCT(15 as measure, NULL as col1, NULL as col2)
])

 WHERE 1 = 1 
  AND
  col1 IS NULL
	
	GROUP BY	
		col2

) AS `virtual_table`
 LIMIT 1000

ADDITIONAL INFORMATION

• Fixes Virtual Dataset get_filters ignores IS NULL and IS NOT NULL filters #33294
• Required feature flags: ENABLE_TEMPLATE_PROCESSING = True

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Unclear Boolean Expression Layout ▹ view

Files scanned

​

File Path	Reviewed
superset/jinja_context.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[korbit-ai]

Unclear Boolean Expression Layout

Tell me more

What is the issue?

The boolean expression is split across multiple lines in a way that makes it harder to read, with the 'or' operator dangling on its own line.

Why this matters

Having logical operators on separate lines makes it difficult to quickly scan and understand the boolean logic flow.

Suggested change ∙ Feature Preview

            and (val or op in (FilterOperator.IS_NULL.value,
                                FilterOperator.IS_NOT_NULL.value))

Provide feedback to improve future suggestions

💬 Looking for more details? Reply to this comment to chat with Korbit.

[Prokos]

I disagree that this is an improvement, but happy to change if a more senior contributor does think so!

[korbit-ai]

Makes sense. The current format does provide clear logical grouping. Let's keep it as is unless senior contributors indicate otherwise.

[Prokos]

Not me talking to an AI: after the ruff-format fix maybe the final logic is a bit too expanded but I'll leave it to a reviewer to decide on what formatting should be used. At least now it's passing the test.

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.28%. Comparing base (2a97a6e) to head (28bd367).

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #33296       +/-   ##
===========================================
+ Coverage        0   83.28%   +83.28%     
===========================================
  Files           0      553      +553     
  Lines           0    39936    +39936     
===========================================
+ Hits            0    33259    +33259     
- Misses          0     6677     +6677     

Flag	Coverage Δ
hive	48.28% <ø> (?)
mysql	75.46% <ø> (?)
postgres	75.53% <ø> (?)
presto	52.76% <ø> (?)
python	83.28% <ø> (?)
sqlite	75.03% <ø> (?)
unit	61.36% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Prokos]

@sfirke Pre-commit now runs successfully locally :)

[sfirke]

I tagged a few committers who I believe are familiar with the Jinja internals. Hopefully one of them can take a quick look and we can get this merged!