Skip to content

fix: guard potential null derefs and remove dead branches#42358

Open
msyavuz wants to merge 1 commit into
apache:masterfrom
msyavuz:msyavuz/fix/backend-null-safety-cleanups
Open

fix: guard potential null derefs and remove dead branches#42358
msyavuz wants to merge 1 commit into
apache:masterfrom
msyavuz:msyavuz/fix/backend-null-safety-cleanups

Conversation

@msyavuz

@msyavuz msyavuz commented Jul 23, 2026

Copy link
Copy Markdown
Member

SUMMARY

Small robustness/cleanup pass in a few backend paths surfaced by static analysis:

  • databricks validate_parameters: connect_args was only bound inside the if extra := json.loads(...) block, so an empty extra raised UnboundLocalError at connect_args.get("http_path"). Now initialized to {} up front.
  • query_cache_manager: inside the if cache_value := ... block cache_value is always truthy, so is_cached = cache_value is not None and the else None branch of cache_dttm were redundant/dead. Simplified.
  • query_context_factory: made the None-safety of filter_to_remove explicit before is_adhoc_column.
  • presto expand_data: unnested_rows was only assigned inside a conditional; initialized before the loop so it's always bound.

TESTING INSTRUCTIONS

No behavior change except the databricks fix (empty extra no longer raises). Covered by existing db_engine_specs and query-context unit tests.

ADDITIONAL INFORMATION

  • Has associated issue: No
  • Required feature flags: No
  • Changes UI: No
  • Includes DB Migration: No
  • Introduces new feature or API: No
  • Removes existing feature or API: No

@dosubot dosubot Bot added the change:backend Requires changing the backend label Jul 23, 2026
@bito-code-review

bito-code-review Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #ec0c8a

Actionable Suggestions - 0
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/db_engine_specs/databricks.py - 1
Review Details
  • Files reviewed - 4 · Commit Range: 166563e..166563e
    • superset/common/query_context_factory.py
    • superset/common/utils/query_cache_manager.py
    • superset/db_engine_specs/databricks.py
    • superset/db_engine_specs/presto.py
  • Files skipped - 0
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@netlify

netlify Bot commented Jul 23, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 7f4d017
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a637f0c0195380008757cfc
😎 Deploy Preview https://deploy-preview-42358--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added risk:db-migration PRs that require a DB migration size/S api Related to the REST API doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages risk:ci-script PR modifies scripts that execute in CI (supply chain risk) labels Jul 24, 2026
- databricks: bind connect_args before the optional extra block in
  validate_parameters so it can't be referenced unbound when extra is empty
- query_cache_manager: drop redundant None checks inside the truthy
  cache_value block (is_cached, cache_dttm)
- query_context_factory: make the None-safety of filter_to_remove explicit
  before is_adhoc_column
- presto: initialize unnested_rows before the expand loop so it is always
  bound
@msyavuz
msyavuz force-pushed the msyavuz/fix/backend-null-safety-cleanups branch from 90b725f to 7f4d017 Compare July 24, 2026 15:04
@github-actions github-actions Bot removed risk:db-migration PRs that require a DB migration api Related to the REST API doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages risk:ci-script PR modifies scripts that execute in CI (supply chain risk) labels Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.27%. Comparing base (67c3fea) to head (7f4d017).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
superset/common/query_context_factory.py 0.00% 0 Missing and 1 partial ⚠️
superset/db_engine_specs/databricks.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42358      +/-   ##
==========================================
- Coverage   65.27%   65.27%   -0.01%     
==========================================
  Files        2794     2794              
  Lines      157520   157522       +2     
  Branches    36038    36038              
==========================================
+ Hits       102815   102816       +1     
- Misses      52729    52730       +1     
  Partials     1976     1976              
Flag Coverage Δ
hive 38.42% <0.00%> (-0.01%) ⬇️
mysql 57.62% <66.66%> (-0.01%) ⬇️
postgres 57.66% <66.66%> (-0.01%) ⬇️
presto 40.35% <33.33%> (-0.01%) ⬇️
python 59.07% <66.66%> (-0.01%) ⬇️
sqlite 57.29% <66.66%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bito-code-review

bito-code-review Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #b58dfc

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset/common/utils/query_cache_manager.py - 1
    • Cache corruption resilience lost · Line 211-211
      Direct key access `cache_value["dttm"]` replaces the original `cache_value["dttm"] if cache_value is not None else None`. Since `cache_value` is already guarded non-None, the old ternary was redundant — but the safe `.get("dttm")` fallback is lost. If a cache entry is missing the "dttm" key (e.g., corrupted entry or legacy format), the `KeyError` is caught at line 223, leaving `is_loaded=True` but `cache_dttm` unset — an inconsistent partial initialization that can cause downstream failures.
Review Details
  • Files reviewed - 4 · Commit Range: 7f4d017..7f4d017
    • superset/common/query_context_factory.py
    • superset/common/utils/query_cache_manager.py
    • superset/db_engine_specs/databricks.py
    • superset/db_engine_specs/presto.py
  • Files skipped - 0
  • Tools
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:backend Requires changing the backend size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant