Skip to content

Support SQLAlchemy 2.0 in FB storage extensions#5203

Open
LeoMoonStar wants to merge 1 commit into
facebook:mainfrom
LeoMoonStar:export-D104875016
Open

Support SQLAlchemy 2.0 in FB storage extensions#5203
LeoMoonStar wants to merge 1 commit into
facebook:mainfrom
LeoMoonStar:export-D104875016

Conversation

@LeoMoonStar
Copy link
Copy Markdown

Summary:
The Meta-internal Ax storage extensions in ax/fb/storage/ have two SA 2.0 incompatibilities not present in the OSS surface: a raw SQL string passed to session.execute in fb sqa_store db.py (SA 2.0 requires text() wrapping), and external_store.py uses Connection.execute() for writes without explicit transaction (SA 2.0 removed implicit autocommit, so writes were silently rolling back), uses string-keyed Row indexing (SA 2.0 requires row._mapping[key]), and consumes a Result generator outside the connection context (SA 2.0 closes the Result on connection close).

This diff wraps SHOW DATABASES with text(), switches _write to engine.begin() for transactional commit, migrates _decode_row to row._mapping access, and materializes the read_raw_data result list inside the with conn block. Adds tests_sa2 dual-version Buck targets for fb sqa_store, fb external_store, and fb prod_tests, plus a SQLAlchemy2CompatTest smoke test that exercises the libfb.py.db_locator -> creator -> engine -> session -> SELECT 1 path and asserts EXPECTED_SA_MAJOR.

Differential Revision: D104875016

Summary:
The Meta-internal Ax storage extensions in ax/fb/storage/ have two SA 2.0 incompatibilities not present in the OSS surface: a raw SQL string passed to session.execute in fb sqa_store db.py (SA 2.0 requires text() wrapping), and external_store.py uses Connection.execute() for writes without explicit transaction (SA 2.0 removed implicit autocommit, so writes were silently rolling back), uses string-keyed Row indexing (SA 2.0 requires row._mapping[key]), and consumes a Result generator outside the connection context (SA 2.0 closes the Result on connection close).

This diff wraps SHOW DATABASES with text(), switches _write to engine.begin() for transactional commit, migrates _decode_row to row._mapping access, and materializes the read_raw_data result list inside the with conn block. Adds tests_sa2 dual-version Buck targets for fb sqa_store, fb external_store, and fb prod_tests, plus a SQLAlchemy2CompatTest smoke test that exercises the libfb.py.db_locator -> creator -> engine -> session -> SELECT 1 path and asserts EXPECTED_SA_MAJOR.

Differential Revision: D104875016
@meta-cla meta-cla Bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 19, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented May 19, 2026

@LeoMoonStar has exported this pull request. If you are a Meta employee, you can view the originating Diff in D104875016.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.38%. Comparing base (bd2ff97) to head (bbea32b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5203   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         617      617           
  Lines       69615    69615           
=======================================
  Hits        67100    67100           
  Misses       2515     2515           

☔ View full report in Codecov by Sentry.
📢 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.

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

Labels

CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants