fix: Pre-query normalization with custom SQL

[michael-s-molina]

SUMMARY

This PR fixes an issue with pre-query normalization when dealing with custom SQL. Previously, a custom SQL that had a label that does not match any column name would throw the following error:

superset/superset/connectors/sqla/models.py", line 1691, in _normalize_prequery_result_type
    column_ = columns_by_name[dimension]
KeyError: 'custom_sql'

Given function docs:

Convert a prequery result type to its equivalent Python type.

Some databases like Druid will return timestamps as strings, but do not perform
automatic casting when comparing these strings to a timestamp. For cases like
this we convert the value via the appropriate SQL transform.

We simply skip the conversion of custom SQL types as we cannot determine if they are temporal or not currently.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2024-09-25.at.14.13.47.mov

TESTING INSTRUCTIONS

Repro the steps shown in the video using a db engine spec with allows_subqueries = False

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

All modified and coverable lines are covered by tests ✅

Project coverage is 83.89%. Comparing base (76d897e) to head (785069d).
Report is 1094 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30389       +/-   ##
===========================================
+ Coverage   60.48%   83.89%   +23.41%     
===========================================
  Files        1931      533     -1398     
  Lines       76236    38519    -37717     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32317    -13797     
+ Misses      28017     6202    -21815     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.99% <0.00%> (-0.17%)	⬇️
javascript	?
mysql	76.87% <100.00%> (?)
postgres	76.94% <100.00%> (?)
presto	53.50% <0.00%> (-0.31%)	⬇️
python	83.89% <100.00%> (+20.41%)	⬆️
sqlite	76.39% <100.00%> (?)
unit	60.66% <100.00%> (+3.04%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[villebro]

@michael-s-molina I'm trying to understand when this would happen. Could this have something to do with BaseEngineSpec.make_label_compatible changing the column alias?

[michael-s-molina]

@michael-s-molina I'm trying to understand when this would happen. Could this have something to do with BaseEngineSpec.make_label_compatible changing the column alias?

@villebro This happens when your engine spec has allows_subqueries = False and you apply a series limit. I updated the PR description with a video.

[villebro]

Fix LGTM, although I'm not convinced that method should be there in the first place. I'd almost prefer to follow Elon's "delete, delete, delete" mantra here and remove the whole method instead of fixing this bug, but in the interest of stabilizing the codebase maybe not right now. However, maybe we could start using some convention for annotating code that should be removed/cleaned up during breaking windows? If we don't start cleaning up stuff like this we'll never get out of the tech debt hole we're in..

[michael-s-molina]

However, maybe we could start using some convention for annotating code that should be removed/cleaned up during breaking windows? If we don't start cleaning up stuff like this we'll never get out of the tech debt hole we're in..

@villebro We generally use the @deprecated annotation but I don't have enough context about this function to actually mark it as deprecated. Maybe @betodealmeida will know more and we could add the annotation in a follow-up if it's a valid case.

[villebro]

However, maybe we could start using some convention for annotating code that should be removed/cleaned up during breaking windows? If we don't start cleaning up stuff like this we'll never get out of the tech debt hole we're in..

We generally use the @deprecated annotation but I don't have enough context about this function to actually mark it as deprecated. Maybe @betodealmeida will know more and we could add the annotation in a follow-up if it's a valid case.

Good point, we can reuse that even for non-public methods. I'll keep that in mind in subsequent PRs 👍