[Bugfix] Remove prequery properties from query_obj

[villebro]

CATEGORY

Choose one

• Bug Fix
• Refactor

SUMMARY

TL;DR:

There is a bug in how the prequeries property of query_obj is constructed in sqla/models.py, which causes cache misses if there are more than one filter selected in FilterBox. This PR fixes that bug without introducing any (user facing) functional changes.

Long explanation:

When non-join based prequeries were introduced in #4163, the properties is_prequery and prequeries were introduced into query_obj. This was done to be able to produce "Top N" filtering for engines that don't support joins, namely Druid (and later Pinot). However, prequeries are not actually needed in the query_obj (and later cache_dict), as they are only an intermediary construct that can be deduced from the query_obj. Furthermore, prequeries were added to query_obj in sqla_models.py, making the calls non-idempotent and non-deterministic. Ideally query_obj should never be mutated after it is constructed in viz.py, as all the information necessary for the query should already be known at that time. This caused a caching bug in Filterbox, which runs as many queries as there are filters, and erroneously interpreted the previous queries as prequeries, causing bloated prequeries properties in the query_obj.

When Pinot support was introduced in #6719, two named tuples (SqlaQuery and QueryStrExtended) were introduced in sqla/models.py to facilitate returning more context for how a query that produces the resulting DataFrame were produced. In the current codebase, this is the correct placeholder for prequeries. This PR removes both is_prequery and prequeries from query_obj, and adds prequeries into the SqlaQuery and QueryStrExtended objects, enabling the functionality introduced in #4163.

Functional changes:

• Removed prequeries and is_prequery from query_obj.
• Moved prequery related handling from query_obj to SqlaQuery and QueryStrExtended, ensuring that query_obj stays unmutated during processing in sqla/models.py.

Other refactoring:

• Renamed property inner_joins in BaseEngineSpec to allows_joins (currently not necessary to distinguish between inner/outer/full joins). Also harmonized other similarly named properties, such as allows_subqueries and allows_column_aliases.
• Added typing to SqlaQueryand QueryStrExtended.
• Moved some query formatting from core.py to sqla/models.py.

TEST PLAN

• CI
• Tested locally by setting allows_joins to both True and False on sqlite
• Verified that no more cache misses occur when running Filterbox with more than one filter.

ADDITIONAL INFORMATION

• Has associated issue: Fixes Filter Box Caching Incorrectly for Multi-Query Use Case #7666

REVIEWERS

@smokemonster99

[codecov-io]

Codecov Report

Merging #7896 into master will increase coverage by 8.88%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7896      +/-   ##
==========================================
+ Coverage   56.95%   65.83%   +8.88%     
==========================================
  Files         354      461     +107     
  Lines       10563    22210   +11647     
  Branches     2425     2425              
==========================================
+ Hits         6016    14622    +8606     
- Misses       4426     7467    +3041     
  Partials      121      121

Impacted Files	Coverage Δ
superset/viz.py	71.77% <ø> (ø)
superset/connectors/druid/models.py	82.2% <ø> (ø)
superset/common/query_object.py	27.77% <ø> (ø)
superset/connectors/sqla/models.py	83.5% <100%> (ø)
superset/db_engine_specs/druid.py	100% <100%> (ø)
superset/db_engine_specs/base.py	87.98% <100%> (ø)
superset/db_engine_specs/gsheets.py	100% <100%> (ø)
superset/models/core.py	82.95% <100%> (ø)
superset/db_engine_specs/pinot.py	68.96% <100%> (ø)
superset/views/core.py	71.93% <100%> (ø)
... and 107 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 4568b2a...b2e03e1. Read the comment docs.

[smokemonster99]

I tested in my dev environment on v33.0 and it looks good. LGTM

[villebro]

@smokemonster99 looking more closely this bug rquires delicate fixing, as the logic causing it is quite intricate. Will probably take a few days + a thorough round of review. In the meantime this WIP hack or your fix should be ok for your use case.

[villebro]

@smokemonster99 this is now ready; Can you test this in your env to make sure everything works as intended?

[mistercrunch]

Thanks for digging deep on this one. This LGTM, but would like if @betodealmeida @agrawaldevesh could take a look.

[villebro]

Yeah, this was some serious rabbit hole stuff 😅

[smokemonster99]

@villebro I will be traveling for meetings next week, I’ll see if I can get to it mid or late next week if you haven’t merged by then.

[villebro]

@smokemonster99 no prob and thanks for bringing this to our attention, good catch!

[mistercrunch]

Oh wow, the new NamedTuple are so much better, always hated the way the old ones were delcared...

[mistercrunch]

Did a pass and it LGTM, feel free to merge AFAIC

[villebro]

Thanks for the review @mistercrunch ; I think this turned out to be less invasive than I had originally thought, so will go ahead and merge.