Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cross-filters): add support for temporal filters #16139

Merged
merged 11 commits into from
Aug 10, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented Aug 9, 2021

SUMMARY

Add support for time grain based native-filters and cross-filters. The frontend work is done in apache-superset/superset-ui#1281 (this PR can be merged independently of that PR, as it only adds backend support for the feature). Other changes:

  • Add new TypedDicts for AdhocFilterClause and QueryObjectFilterClause and implement necessary changes where we were previously assuming filters were Dict[str, Any].
  • Break up to_adhoc function into simple_filter_to_adhoc and form_data_to_adhoc, as the previous implementation was mixing filter objects with form data. Also fix affected tests + remove a mock that is deemed unnecessary.

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

Comment on lines +1065 to +1066
time_grain = extras.get("time_grain_sqla")
dttm_col = columns_by_name.get(granularity) if granularity else None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are moved up in the code as they are needed earlier now

Comment on lines -88 to -100
def mock_to_adhoc(filt, expressionType="SIMPLE", clause="where"):
result = {"clause": clause.upper(), "expressionType": expressionType}

if expressionType == "SIMPLE":
result.update(
{"comparator": filt["val"], "operator": filt["op"], "subject": filt["col"]}
)
elif expressionType == "SQL":
result.update({"sqlExpression": filt[clause]})

return result


Copy link
Member Author

@villebro villebro Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a mock like this felt unnecessary and dangerous, as it's mostly the same as the original function + the logic associated with conversion between regular and adhoc metrics is already brittle. Therefore this mock is removed and the assertions updated to reflect what is actually expected to be returned when running the full chain of convert_legacy_filters_into_adhoc, merge_extra_filters and split_adhoc_filters_into_base_filters.

@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #16139 (ba0121b) into master (ddb5005) will decrease coverage by 0.12%.
The diff coverage is 86.06%.

❗ Current head ba0121b differs from pull request most recent head 2ff35a8. Consider uploading reports for the commit 2ff35a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16139      +/-   ##
==========================================
- Coverage   76.83%   76.71%   -0.13%     
==========================================
  Files         995      996       +1     
  Lines       52884    52954      +70     
  Branches     6721     6725       +4     
==========================================
- Hits        40636    40626      -10     
- Misses      12023    12103      +80     
  Partials      225      225              
Flag Coverage Δ
hive ?
mysql 81.58% <87.50%> (+0.04%) ⬆️
postgres 81.61% <87.50%> (+<0.01%) ⬆️
presto 81.44% <87.50%> (+0.05%) ⬆️
python 81.87% <87.50%> (-0.25%) ⬇️
sqlite 81.21% <87.50%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
.../src/components/dataViewCommon/TableCollection.tsx 100.00% <ø> (ø)
...ontrols/DndColumnSelectControl/DndFilterSelect.tsx 45.07% <ø> (ø)
...nents/controls/MetricControl/AdhocMetricOption.jsx 75.00% <ø> (ø)
...s/controls/MetricControl/MetricDefinitionValue.jsx 80.00% <ø> (ø)
...mponents/controls/MetricControl/MetricsControl.jsx 82.94% <ø> (ø)
superset/utils/profiler.py 47.05% <47.05%> (ø)
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx 41.78% <50.00%> (-0.29%) ⬇️
...plore/components/controls/OptionControls/index.tsx 88.88% <60.00%> (-1.54%) ⬇️
superset/initialization/__init__.py 87.72% <75.00%> (-0.19%) ⬇️
superset/extensions.py 92.68% <85.71%> (-0.66%) ⬇️
... and 18 more

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 ddb5005...2ff35a8. Read the comment docs.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! Left some nit picking comments, looks good after conflicts are fixed

superset/utils/core.py Outdated Show resolved Hide resolved
superset/utils/core.py Show resolved Hide resolved
superset/utils/core.py Show resolved Hide resolved
superset/typing.py Show resolved Hide resolved
superset/connectors/sqla/models.py Show resolved Hide resolved
@villebro
Copy link
Member Author

villebro commented Aug 9, 2021

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2021

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

@villebro
Copy link
Member Author

FYI this doesn't work properly with SQLite yet due to the dates being strings. If it's important to add support for SQLite I can add that - otherwise I propose leaving it to a follow-up PR for a rainy day.

@dpgaspar dpgaspar merged commit 63ace7b into apache:master Aug 10, 2021
@rusackas rusackas deleted the villebro/table-filter branch August 10, 2021 16:26
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rosemarie-chiu
Copy link
Contributor

🏷 2021.31

stevenuray pushed a commit to preset-io/superset that referenced this pull request Aug 11, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test

(cherry picked from commit 63ace7b)
@villebro villebro added the v1.3 label Aug 12, 2021
villebro added a commit that referenced this pull request Aug 16, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test

(cherry picked from commit 63ace7b)
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test

(cherry picked from commit 63ace7b)
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test

(cherry picked from commit 63ace7b)
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* feat(cross-filters): add support for temporal filters

* fix test

* make filter optional

* remove mocks

* fix more tests

* remove unnecessary optionality

* fix even more tests

* bump superset-ui

* add isExtra to schema

* address comments

* fix presto test

(cherry picked from commit f042910)
@mistercrunch mistercrunch added 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2021.31 preset-io size/L v1.3 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants