Skip to content

fix(import): skip redundant has_table check when dataset import has no data URI - #43808

Open
Kunal8954 wants to merge 2 commits into
apache:masterfrom
Kunal8954:fix/dataset-import-has-table-check
Open

fix(import): skip redundant has_table check when dataset import has no data URI#43808
Kunal8954 wants to merge 2 commits into
apache:masterfrom
Kunal8954:fix/dataset-import-has-table-check

Conversation

@Kunal8954

Copy link
Copy Markdown

Summary

Fixes #43764 — bulk dataset imports (e.g. 293 datasets) time out on the gunicorn worker.

Root cause

For every dataset being imported, Database.has_table() was called unconditionally. This opens a live connection to the target database and runs a schema-introspection query — a real network round trip. Its result was only ever used to decide whether to call load_data(), which is itself a no-op unless the dataset's config carries an inline data URI (uncommon — most bulk metadata imports don't). So for a typical bulk import of dataset definitions, every single dataset paid for one wasted database round trip.

Fix

Only call has_table() when data_uri is actually present, since that's the only case where its result is ever used.

Testing

Added two unit tests: one asserting has_table is not called when there's no data URI, one asserting it is called when there is one.

…o data URI

Database.has_table() opens a live connection and runs a schema-
introspection query against the target database. Its result was only
ever consulted to decide whether to call load_data(), which itself is
already a no-op when a dataset's config has no 'data' URI. For bulk
imports of dataset metadata (the common case, with no inline data),
this meant one unconditional, unnecessary round trip to the target
database per dataset — a major contributor to large imports (e.g.
293 datasets) timing out on the gunicorn worker.

Only call has_table() when a data URI is actually present.

Closes: apache#43764
@bito-code-review

bito-code-review Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #0b7c1b

Actionable Suggestions - 0
Additional Suggestions - 1
  • tests/unit_tests/datasets/commands/importers/v1/import_test.py - 1
    • Duplicate test setup · Line 776-864
      `test_import_dataset_skips_has_table_check_without_data_uri` and `test_import_dataset_checks_has_table_with_data_uri` are near-identical except for the `data` key and the assertion. Consider parametrizing over `(has_data, expected_calls)` to avoid ~45 duplicated lines of setup (database creation, config dict, `import_dataset` call) that can drift apart.
Review Details
  • Files reviewed - 2 · Commit Range: bc03ca3..bc03ca3
    • superset/commands/dataset/importers/v1/utils.py
    • tests/unit_tests/datasets/commands/importers/v1/import_test.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

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.46%. Comparing base (e8540b7) to head (7a2e170).
⚠️ Report is 91 commits behind head on master.

Files with missing lines Patch % Lines
superset/commands/dataset/importers/v1/utils.py 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #43808      +/-   ##
==========================================
+ Coverage   79.35%   79.46%   +0.11%     
==========================================
  Files        2893     2896       +3     
  Lines      167502   168275     +773     
  Branches    38801    38925     +124     
==========================================
+ Hits       132920   133719     +799     
+ Misses      32086    32055      -31     
- Partials     2496     2501       +5     
Flag Coverage Δ
hive 37.67% <0.00%> (-0.16%) ⬇️
mysql 57.38% <0.00%> (-0.19%) ⬇️
postgres 57.41% <0.00%> (-0.20%) ⬇️
presto 39.56% <0.00%> (-0.17%) ⬇️
python 83.99% <62.50%> (+0.18%) ⬆️
sqlite 57.12% <0.00%> (-0.18%) ⬇️
unit 74.62% <62.50%> (+0.34%) ⬆️

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.


import_dataset(config)

has_table.assert_called_once()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If has_table() returns False, this import should still load the supplied data, but this test returns True and never reaches load_data. An inverted/reindented condition would therefore pass the new coverage while skipping initialization of a missing table. Could this patch load_data and assert one call for the absent-table case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point, the has_table=True case never reached load_data so it proved
very little. Patched load_data in both tests now: not called when the table
exists, called once when it doesn't. Checked it catches the inverted
condition you described — flipping not table_exists fails both new
assertions.

@bito-code-review

Copy link
Copy Markdown
Contributor

The current implementation correctly optimizes the import process by wrapping the has_table check in an if data_uri: block, ensuring the database round trip only occurs when data actually needs to be loaded. The test test_import_dataset_skips_has_table_check_without_data_uri correctly asserts that has_table is not called when data_uri is absent.

To address the case where has_table() returns False (meaning the table is missing), you can add a test case that mocks has_table to return False and asserts that load_data is called. This ensures that when data is provided but the table is missing, the import proceeds as expected.

tests/unit_tests/datasets/commands/importers/v1/import_test.py

def test_import_dataset_calls_load_data_when_table_missing(mocker: MockerFixture, session: Session) -> None:
    mocker.patch.object(security_manager, "can_access", return_value=True)
    has_table = mocker.patch.object(Database, "has_table", return_value=False)
    load_data = mocker.patch("superset.commands.dataset.importers.v1.utils.load_data")

    config = { ... } # Use a config with a data_uri
    import_dataset(config)

    has_table.assert_called_once()
    load_data.assert_called_once()

Patch load_data in both data-URI tests and assert on it: not called when
has_table reports the table already exists, called once when it doesn't.
Without this, an inverted or misindented condition would skip loading a
missing table while still satisfying the has_table assertion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015kAvbZ6SeKgp6jbGcvpXSh
@netlify

netlify Bot commented Sep 5, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 7a2e170
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a9b7bf20afa8400084ba0f1
😎 Deploy Preview https://deploy-preview-43808--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.

@bito-code-review

bito-code-review Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #855b14

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: bc03ca3..7a2e170
    • tests/unit_tests/datasets/commands/importers/v1/import_test.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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dataset import timeout

2 participants