Skip to content

fix(sql): use AST for disallowed function checks - #43641

Open
wangfenjin wants to merge 2 commits into
apache:masterfrom
wangfenjin:fix/sql-executor-function-denylist-ast
Open

fix(sql): use AST for disallowed function checks#43641
wangfenjin wants to merge 2 commits into
apache:masterfrom
wangfenjin:fix/sql-executor-function-denylist-ast

Conversation

@wangfenjin

@wangfenjin wangfenjin commented Aug 28, 2026

Copy link
Copy Markdown

SUMMARY

The unified SQL executor currently stringifies parsed SQL and checks each configured disallowed function with substring containment. For PostgreSQL, the default version denylist therefore rejects identifiers such as conversion and custom_skill_versions, as well as literals and comments containing that text.

Reuse the existing SQLScript.check_functions_present AST matcher so the denylist applies to actual function calls. This keeps version() and pg_catalog.version() blocked without weakening the configured check, while avoiding false positives in identifiers, literals, and comments.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; this is a backend SQL validation change.

TESTING INSTRUCTIONS

  1. Run pytest tests/unit_tests/sql/execution/test_executor.py -k disallowed_functions.
  2. Verify version() and pg_catalog.version() are reported as disallowed.
  3. Verify conversion, quoted identifiers, custom_skill_versions, string literals, and comments do not produce false positives.
  4. Run pre-commit run against the staged files.

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

@dosubot dosubot Bot added change:backend Requires changing the backend sqllab Namespace | Anything related to the SQL Lab labels Aug 28, 2026
@bito-code-review

bito-code-review Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #0e3cf4

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 288e551..288e551
    • superset/sql/execution/executor.py
    • tests/unit_tests/sql/execution/test_executor.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 an incremental AI Review.

  • /review full - 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

Comment thread superset/sql/execution/executor.py Outdated
@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. The current implementation in the diff uses a set comprehension that calls script.check_functions_present({func}) for every function in engine_disallowed, which triggers redundant AST traversals.

To resolve this, you should compute the set of all functions present in the script once and then find the intersection with the denylist. Assuming script provides a way to get all present functions (e.g., script.get_all_functions()), the implementation would look like this:

        # Compute present functions once
        present_functions = script.get_all_functions()
        found = {func for func in engine_disallowed if func in present_functions}

If script.get_all_functions() is not available, you should expose a method that performs a single AST traversal to collect all function names, then perform the intersection.

There are no other comments on this PR to address.

superset/sql/execution/executor.py

# Compute present functions once
        present_functions = script.get_all_functions()
        found = {func for func in engine_disallowed if func in present_functions}

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.19%. Comparing base (12cd259) to head (4c2b2ea).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #43641   +/-   ##
=======================================
  Coverage   79.19%   79.19%           
=======================================
  Files        2879     2879           
  Lines      165870   165872    +2     
  Branches    38352    38350    -2     
=======================================
+ Hits       131354   131356    +2     
  Misses      32039    32039           
  Partials     2477     2477           
Flag Coverage Δ
hive 37.94% <33.33%> (+<0.01%) ⬆️
mysql 57.67% <50.00%> (+<0.01%) ⬆️
postgres 57.70% <50.00%> (-0.01%) ⬇️
presto 39.85% <33.33%> (+<0.01%) ⬆️
python 83.74% <100.00%> (+<0.01%) ⬆️
sqlite 57.39% <50.00%> (+<0.01%) ⬆️
unit 73.96% <100.00%> (+<0.01%) ⬆️

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.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Aug 28, 2026
@wangfenjin

Copy link
Copy Markdown
Author

@bito-code-review review

@bito-code-review

bito-code-review Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #65ceff

Actionable Suggestions - 0
Review Details
  • Files reviewed - 3 · Commit Range: 288e551..4c2b2ea
    • superset/sql/execution/executor.py
    • superset/sql/parse.py
    • tests/unit_tests/sql/execution/test_executor.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 an incremental AI Review.

  • /review full - 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/L sqllab Namespace | Anything related to the SQL Lab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant