refactor: do not cascade to SqlMetric#42221
Conversation
Code Review Agent Run #e81c94Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42221 +/- ##
==========================================
- Coverage 65.19% 65.19% -0.01%
==========================================
Files 2768 2768
Lines 156081 156086 +5
Branches 35719 35718 -1
==========================================
+ Hits 101754 101757 +3
- Misses 52365 52367 +2
Partials 1962 1962
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
231b9aa to
6f3fb56
Compare
Code Review Agent Run #1b66afActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #63e27aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
7831082 to
e5df73d
Compare
|
@hy144328 thanks for the PR. Mind adding more details in your PR description. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #5f6135
Actionable Suggestions - 1
-
tests/integration_tests/datasets/api_tests.py - 1
- Redundant session.add_all call · Line 126-126
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/connectors/sqla/models.py - 1
- Missing session registration for metrics · Line 404-404
Review Details
-
Files reviewed - 9 · Commit Range:
6f3fb56..e5df73d- pytest.ini
- superset/commands/dataset/duplicate.py
- superset/connectors/sqla/models.py
- superset/examples/birth_names.py
- superset/examples/world_bank.py
- tests/integration_tests/conftest.py
- tests/integration_tests/datasets/api_tests.py
- tests/integration_tests/fixtures/energy_dashboard.py
- tests/integration_tests/sqla_models_tests.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
| if columns: | ||
| table.columns = columns | ||
| if metrics: | ||
| db.session.add_all(metrics) |
There was a problem hiding this comment.
The explicit db.session.add_all(metrics) is semantically dead code. SQLAlchemy's cascade="all" includes save-update cascade, which automatically persists child SqlMetric objects when db.session.add(table) is called on line 128. This creates inconsistent treatment compared to how columns is handled on line 124, which correctly relies solely on cascade. Remove this line to maintain consistency and reflect correct understanding of ORM behavior.
Code Review Run #5f6135
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
cascade_backrefs is set to False.
TableColumn is handled in a separate PR.
There was a problem hiding this comment.
The suggestion to remove db.session.add_all(metrics) is technically correct if the SqlMetric relationship is configured with cascade="all, delete-orphan" or similar save-update cascades. In such cases, adding the parent table object to the session automatically persists the associated metrics collection, making the explicit add_all call redundant. Removing it improves consistency with how the columns collection is handled in the same function.
tests/integration_tests/datasets/api_tests.py
if columns:
table.columns = columns
if metrics:
db.session.add_all(metrics)
|
Thanks for this, Hans. Bito's thread on Also, sadpandajoe's ask above for more detail in the PR description is still open, would help given this touches cascade semantics in a few places, and we do refer back to PR descriptions in various places for history. Looks like it needs a rebase too. |
3 participants
See #40273 .
SUMMARY
Disable
cascade_backrefsonSqlMetricbecause it is deprecated in Airflow 2.Manually add objects to session that were previously added automatically.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Unit tests:
Integration tests via CI.
ADDITIONAL INFORMATION