feat: add support for generic series limit

[villebro]

SUMMARY

This is the first in a series of multiple PRs that aims to add generic x-axis support. The long-term goal is the following:

• deprecate is_timeseries - a plugin should explicitly add the temporal column to the groupby if it is used as a dimension. This behavior is not changed in this PR, but removes the need for enabling is_timeseries to be able to limit series count.
• deprecate timeseries_limit and timeseries_limit_metric with series_columns, series_limit and series_limit_metric.
• Deprecate groupby in favor of columns.

Existing charts should not be affected - legacy charts will work as before, but V1 charts will emit deprecation warnings when requesting data using the deprecated field names.

Related PR on superset-ui: apache-superset/superset-ui#1356

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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 #16660 (40764d8) into master (092ef5b) will decrease coverage by 0.02%.
The diff coverage is 86.15%.

@@            Coverage Diff             @@
##           master   #16660      +/-   ##
==========================================
- Coverage   76.99%   76.96%   -0.03%     
==========================================
  Files        1007     1007              
  Lines       54133    54155      +22     
  Branches     7374     7374              
==========================================
+ Hits        41678    41681       +3     
- Misses      12215    12234      +19     
  Partials      240      240              

Flag	Coverage Δ
hive	81.31% <86.15%> (-0.05%)	⬇️
javascript	71.30% <ø> (ø)
mysql	81.66% <86.15%> (-0.05%)	⬇️
postgres	81.76% <86.15%> (-0.05%)	⬇️
presto	81.65% <86.15%> (-0.01%)	⬇️
python	82.27% <86.15%> (-0.06%)	⬇️
sqlite	81.33% <86.15%> (-0.05%)	⬇️

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

Impacted Files	Coverage Δ
superset/common/query_actions.py	92.95% <ø> (-0.10%)	⬇️
superset/common/query_context.py	90.74% <ø> (ø)
superset/db_engine_specs/base.py	88.39% <ø> (ø)
superset/connectors/sqla/models.py	87.95% <75.75%> (-1.76%)	⬇️
superset/common/query_object.py	90.62% <96.55%> (-0.05%)	⬇️
superset/charts/schemas.py	100.00% <100.00%> (ø)
superset/db_engine_specs/presto.py	89.95% <0.00%> (-0.42%)	⬇️

---

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 092ef5b...40764d8. Read the comment docs.

[villebro]

OrderedDict is no longer necessary, as dict is guaranteed to be ordered as of Python 3.7.

https://docs.python.org/3.8/library/stdtypes.html

[villebro]

Bycatch: I noticed we weren't checking if the engine allows subqueries

[villebro]

I have no idea what db_engine_spec.time_groupby_inline does, but I can only assume it relates to temporal filters in subqueries. If this turns out not to be the case I can improve the documentation in a subsequent PR.

[villebro]

In hindsight I should not have reordered these in this PR to keep it as easily reviewable as possible (my sincere apologies to reviewers!), but having these non-ordered annoyed me so much I couldn't help myself. The point here is I've added series_columns, series_limit and series_limit_metric and removed timeseries_limit and timeseries_limit_metric as they're picked up by kwargs and handled in the deprecated mapping methods.

[villebro]

Here I'm renaming columns to table_columns, as columns should refer to the actual query object property (I doubt the old columns property is really being actively used).

[ktmud]

Thanks for tackling this! This is long overdue. I'm super excited on seeing what we can do with Superset charts by "downgrading" time columns to a place it really deserves---to be treated the same like any other columns.

[ktmud]

self.series_columns = series_columns or (columns if is_timeseries and metrics else [])

Would this be more Pythonic?

I'm also wondering whether we should have another layer of parameter consolidation before QueryObject to handle all the special overrides & fallbacks for legacy/deprecated parameters (e.g. timeseries_limit vs series_limit, columns vs groupby, sortby vs orderby). By isolating parameter consolidation in the Flask view handler layer, we reduce the number of parameters for QueryObject itself and simply all downstream functions, which may help cleaning up deprecated code faster---all without affecting backward compatibility.

[villebro]

self.series_columns = series_columns or (columns if is_timeseries and metrics else [])

Would this be more Pythonic?

IMO the current implementation is more pythonic/readable, as it doesn't require unnesting the inline logic when reading it. But I'm ok with the proposed snippet, too. I'm curious to hear what @dpgaspar thinks - it would be nice to add something on this to the code style guide.

I'm also wondering whether we should have another layer of parameter consolidation before QueryObject so to handle all the special overrides & fallbacks for legacy/deprecated parameters (e.g. timeseries_limit vs series_limit, columns vs groupby, sortby vs orderby). By isolating parameter consolidation in the Flask view handler layer, we reduce the number of parameters for QueryObject itself and simply all downstream functions, which may help cleaning up deprecated code faster---all without affecting backward compatibility.

I wanted to restrict this PR to the bare minimum amount of changes to contain the blast radius (whenever we're touching this type of core logic there's serious risk of regressions). I absolutely agree we should consolidate more (and should finish the consolidation work before cutting 2.0), but I'd propose doing it across multiple PRs to avoid making one big bang PR.

[dpgaspar]

I have no strong opinions here, but, I do like to use simple ternaries. But current implementation seems more readable to me because of the nested ternary.

[villebro]

timeseries_limit previously defaulted to 15. However, this was always defaulted to 0 in both viz.py and QueryObject, so we never actually used this magic number.

[zhaoyongjie]

LGTM

[villebro]

Merging this to unblock superset-ui development

[john-bodley]

@villebro any reason you marked these fields as deprecated—my understanding is both the old and new name may coexist in the form-data (thought the deprecated fields are renamed)—and not just rename them and add a migration?

[villebro]

@villebro any reason you marked these fields as deprecated—my understanding is both the old and new name may coexist in the form-data (thought the deprecated fields are renamed)—and not just rename them and add a migration?

This is mostly in preparation for Superset 2.0. One reason for deprecating these is to avoid introducing breaking changes right now (I assume some people may be calling both the old and new chart data endpoints for custom use cases). Another reason I didn't yet want to add a db migration for these was to avoid having to do extensive refactoring on viz.py. But I fully intend for us to do this later this year when we start preparing in earnest for 2.0.

[ofekisr]

@villebro you forgot to raise the exception here

#16946 fix it