Skip to content

Conversation

@nikolagigic
Copy link
Contributor

SUMMARY

Execute predicate query string if set to improve the filters loads performance.
#12531

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

issue-12531.mp4

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@nikolagigic
Copy link
Contributor Author

@junlincc junlincc added the !deprecated-label:viz:explore:others Deprecated label - Use explore instead label Feb 8, 2021
@junlincc junlincc added the hold! On hold label Feb 8, 2021
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 10, 2021
@pull-request-size pull-request-size bot added size/S and removed size/M labels Feb 10, 2021
superset/viz.py Outdated

def construct_where(self, query_obj: QueryObjectDict, predicate_string: str) -> str:
if query_obj:
predicate_string = "{} AND {}".format(query_obj, predicate_string)
Copy link
Member

Choose a reason for hiding this comment

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

We probably need to add parenthesis here, if either of these has an OR it will break.

Suggested change
predicate_string = "{} AND {}".format(query_obj, predicate_string)
predicate_string = "({}) AND ({})".format(query_obj, predicate_string)

@codecov-io
Copy link

codecov-io commented Feb 10, 2021

Codecov Report

Merging #12998 (fd0d1f6) into master (76c225d) will decrease coverage by 2.27%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12998      +/-   ##
==========================================
- Coverage   69.14%   66.86%   -2.28%     
==========================================
  Files        1025      490     -535     
  Lines       48765    28863   -19902     
  Branches     5188        0    -5188     
==========================================
- Hits        33718    19300   -14418     
+ Misses      14913     9563    -5350     
+ Partials      134        0     -134     
Flag Coverage Δ
cypress ?
javascript ?
python 66.86% <14.28%> (-0.76%) ⬇️

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

Impacted Files Coverage Δ
superset/viz.py 58.86% <14.28%> (-0.19%) ⬇️
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/db_engine_specs/hive.py 73.84% <0.00%> (-16.93%) ⬇️
superset/dataframe.py 91.66% <0.00%> (-8.34%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
superset/connectors/sqla/models.py 84.55% <0.00%> (-6.00%) ⬇️
superset/dashboards/commands/update.py 83.07% <0.00%> (-5.61%) ⬇️
... and 570 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 76c225d...fd0d1f6. Read the comment docs.

superset/viz.py Outdated
Comment on lines 180 to 182
def construct_where(self, query_obj: QueryObjectDict, predicate_string: str) -> str:
if query_obj:
predicate_string = "({}) AND ({})".format(query_obj, predicate_string)
Copy link
Member

Choose a reason for hiding this comment

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

The signature here doesn't seem correct, query_obj here seems to refer to the original where and be Optional[str]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@villebro makes sense. Thanks for the catch!

@junlincc junlincc requested review from eugeniamz and ktmud February 15, 2021 16:37
@junlincc
Copy link
Member

@eugeniamz @ktmud requesting review to make sure this change is okay with you both.

@ktmud
Copy link
Member

ktmud commented Feb 16, 2021

From the code it seems this only applies to FilterBox, but the screenshot shows histogram. Is it only for demonstration's purpose?

The autocomplete predicate query string is used for this dropdown:

which then calls /filter/<datasource_type>/<datasource_id>/<column> and values_for_column. I'm wondering whether we should just reuse this API for FilterBox, too?

@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jun 11, 2021
@stale stale bot closed this Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!deprecated-label:viz:explore:others Deprecated label - Use explore instead hold! On hold inactive Inactive for >= 30 days size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants