fix(generic-chart-axes): apply time filter on all target column types

[villebro]

SUMMARY

When the generic chart axes feature flag is enabled, temporal filters can only be applied on numeric target columns, causing problems for databases such as Druid which stores temporal values in LONG format. To fix this, we should short circuit the handle_single_value inner function in the filter_values_handler of the BaseDatasource class and return the filter value as-is, as that should always be passed to the get_since_until_from_time_range in it's original format here.

As a follow-up, we should clean up these helper functions and add proper unit tests to them (adding unit tests in the current state of this code is fairly difficult).

AFTER

After this fix, the temporal adhoc filter applies cleanly to a non-temporal column, similarly to how id does when disabling the "GENERIC_CHART_AXES" feature flag:

BEFORE

Currently the chart is unable to render, returning a "Must specify a value for filters with comparison operators" error:

TESTING INSTRUCTIONS

1. Gnable "GENERIC_CHART_AXES` feature flag.
2. Go to "Video Game Sales" dashboard and edit the "Most Dominant Platforms" chart.
3. See an error. Set the time filter to "100 years ago" to see that the query generates a relevant filter after the fix has been applied.

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

[codecov]

Codecov Report

Merging #22238 (1d4dab9) into master (22fab5e) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #22238      +/-   ##
==========================================
- Coverage   66.85%   66.84%   -0.01%     
==========================================
  Files        1841     1841              
  Lines       70197    70201       +4     
  Branches     7662     7662              
==========================================
+ Hits        46927    46929       +2     
- Misses      21296    21298       +2     
  Partials     1974     1974              

Flag	Coverage Δ
hive	52.56% <25.00%> (-0.01%)	⬇️
mysql	78.07% <50.00%> (-0.01%)	⬇️
postgres	78.14% <50.00%> (-0.01%)	⬇️
presto	52.45% <25.00%> (-0.01%)	⬇️
python	81.33% <50.00%> (-0.01%)	⬇️
sqlite	76.59% <50.00%> (-0.01%)	⬇️
unit	50.87% <0.00%> (-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	89.32% <ø> (ø)
superset/models/helpers.py	38.19% <0.00%> (-0.09%)	⬇️
superset/connectors/base/models.py	87.20% <100.00%> (+0.07%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[zhaoyongjie]

Thanks for the fixing! I would suggest that move the filter_values_handler method into a helper module and then refactoring it in the future. BTW, we should find out an approach to conversion column type.

[villebro]

Thanks for the fixing! I would suggest that move the filter_values_handler method into a helper module and then refactoring it in the future. BTW, we should find out an approach to conversion column type.

My thoughts exactly - I have an idea of how to get this cleanup started, I'll try to open a PR later in the week.