fix: `IS NULL` filter operator for numeric columns #13496

ktmud · 2021-03-06T01:03:55Z

SUMMARY

Fixes #13229 where filters for numeric columns could not use IS NULL and IS NOT NULL operators in SIMPLE mode.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

Manual testing

Will add more unit tests once SQL query generator was refactored and split into smaller testable functions

ADDITIONAL INFORMATION

ktmud · 2021-03-08T06:15:20Z

superset-frontend/src/explore/exploreUtils.js

    const isString =
      firstValue !== undefined && Number.isNaN(Number(firstValue));
    const quote = isString ? "'" : '';
    const [prefix, suffix] = isMulti ? ['(', ')'] : ['', ''];
    const formattedComparators = comparatorArray.map(
-      val => `${quote}${isString ? val.replace("'", "''") : val}${quote}`,
+      val =>
+        `${quote}${isString ? String(val).replace("'", "''") : val}${quote}`,


This fixes a rare case when <NULL> is used as the first filter value for a numeric column, saving the filter will throw a JS error:

ktmud · 2021-03-08T06:16:05Z

superset/connectors/sqla/models.py

-                    utils.FilterOperator.NOT_IN.value,
-                ):
-                    cond = col_obj.get_sqla_col().in_(eq)
-                    if isinstance(eq, str) and NULL_STRING in eq:


NULL_STRING is already parsed to None in filter_values_handler, so this is not needed.

ktmud · 2021-03-08T06:17:44Z

superset/connectors/sqla/models.py

+                        if eq:
+                            cond = or_(is_null_cond, col_obj.get_sqla_col().in_(eq))
+                        else:
+                            cond = is_null_cond


Do additional check to make the generated where clause cleaner for when there are NULL in the IN list.

Previously it may generate:

WHERE abc in (NULL, 123) OR abc IS NULL

Now it generates:

WHERE abc in (123, ) OR abc IS NOT NULL

why NOT NULL in the last expression ?

Oh, it was a typo... Thanks for noting.

codecov · 2021-03-08T06:38:43Z

Codecov Report

Merging #13496 (c7ad854) into master (9e964f4) will decrease coverage by 0.15%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master   #13496      +/-   ##
==========================================
- Coverage   77.66%   77.51%   -0.16%     
==========================================
  Files         902      902              
  Lines       45873    45873              
  Branches     5598     5596       -2     
==========================================
- Hits        35628    35558      -70     
- Misses      10111    10181      +70     
  Partials      134      134

Flag	Coverage Δ
cypress	`57.44% <100.00%> (-0.02%)`	⬇️
hive	`?`
javascript	`63.58% <100.00%> (-0.01%)`	⬇️
mysql	`80.45% <83.33%> (-0.01%)`	⬇️
postgres	`80.49% <83.33%> (-0.01%)`	⬇️
presto	`80.15% <83.33%> (-0.01%)`	⬇️
python	`80.71% <83.33%> (-0.28%)`	⬇️
sqlite	`80.11% <83.33%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	`90.66% <82.35%> (-0.32%)`	⬇️
superset-frontend/src/explore/exploreUtils.js	`69.81% <100.00%> (-0.75%)`	⬇️
superset/connectors/base/models.py	`90.78% <100.00%> (+0.28%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`74.23% <0.00%> (-16.54%)`	⬇️
...ponents/nativeFilters/FilterBar/CascadePopover.tsx	`95.77% <0.00%> (-1.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e964f4...fbd06e8. Read the comment docs.

bkyryliuk · 2021-03-09T19:23:37Z

superset/connectors/base/models.py

-                values = values[0]
-            else:
-                values = None
+            values = values[0] if values else None


if values could be dangerous, as [],0, None would return false, it would be nice to be a bit more specific in the check.

bkyryliuk · 2021-03-09T19:24:29Z

superset/connectors/sqla/models.py

-                            cond,
-                            col_obj.get_sqla_col()  # pylint: disable=singleton-comparison
-                            == None,
+                if is_list_target:


is it possible to add unit tests here?

Done.

More granular logics could be handled by actual unit tests when we break down the query generator. I also added "revisit:add-tests" label to mark this as a followup item.

bkyryliuk · 2021-03-10T18:05:42Z

superset/connectors/sqla/models.py

+                        if eq:
+                            cond = or_(is_null_cond, col_obj.get_sqla_col().in_(eq))
+                        else:
+                            cond = is_null_cond


why NOT NULL in the last expression ?

bkyryliuk · 2021-03-10T18:08:06Z

Thanks for the fix!

pull-request-size bot added the size/M label Mar 6, 2021

ktmud added the need:tests This PR requires tests label Mar 6, 2021

ktmud force-pushed the is-not-null branch from 9a9701e to 3d7ba90 Compare March 8, 2021 05:30

ktmud marked this pull request as ready for review March 8, 2021 06:13

ktmud commented Mar 8, 2021

View reviewed changes

bkyryliuk reviewed Mar 9, 2021

View reviewed changes

ktmud force-pushed the is-not-null branch from 4b1a19e to 3932bdb Compare March 9, 2021 20:28

ktmud added 7 commits March 9, 2021 16:07

fix: IS NULL filter operator for numeric columns

9488626

More fix and optimization

721cf5d

Force type

4979185

Fix pylint

5e35452

Add test case for filters

97ab41f

Check sql text content

cc2b529

Fix tests

cf80968

ktmud force-pushed the is-not-null branch from 6b899c9 to cf80968 Compare March 10, 2021 02:38

ktmud added 2 commits March 9, 2021 21:15

Fix tests

7a982c6

Add all quotation marks

fbd06e8

ktmud force-pushed the is-not-null branch from 3e16dbf to fbd06e8 Compare March 10, 2021 06:06

bkyryliuk approved these changes Mar 10, 2021

View reviewed changes

ktmud merged commit b9884fb into apache:master Mar 10, 2021

ktmud deleted the is-not-null branch March 10, 2021 18:15

serenajiang pushed a commit to airbnb/superset-fork that referenced this pull request Mar 10, 2021

fix: IS NULL filter operator for numeric columns (apache#13496)

02b4cc5

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

fix: IS NULL filter operator for numeric columns (apache#13496)

94dcfa9

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `IS NULL` filter operator for numeric columns #13496

fix: `IS NULL` filter operator for numeric columns #13496

ktmud commented Mar 6, 2021 •

edited

Loading

ktmud Mar 8, 2021

ktmud Mar 8, 2021

ktmud Mar 8, 2021

bkyryliuk Mar 10, 2021

ktmud Mar 10, 2021

codecov bot commented Mar 8, 2021 •

edited

Loading

bkyryliuk Mar 9, 2021

bkyryliuk Mar 9, 2021

ktmud Mar 9, 2021

bkyryliuk Mar 10, 2021

bkyryliuk commented Mar 10, 2021

fix: IS NULL filter operator for numeric columns #13496

fix: IS NULL filter operator for numeric columns #13496

Conversation

ktmud commented Mar 6, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Mar 10, 2021

fix: `IS NULL` filter operator for numeric columns #13496

fix: `IS NULL` filter operator for numeric columns #13496

ktmud commented Mar 6, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading