GeezerVison is showing github.com — fix: Repair MySQL online store batch_write path by karthikchundi-commits · Pull Request #6802 · feast-dev/feast
Conversation
What this does
The batch_write path of MySQLOnlineStore.online_write_batch (added in #5699) has two bugs that make it unusable:
-
config.online_store.bacth_size is a typo for batch_size. MySQLOnlineStoreConfig is a FeastConfigBaseModel with extra="forbid", so:
- accessing
.bacth_size raises AttributeError → enabling batch_write: true crashes before a single row is written;
- the universal test creator
BatchWriteMySQLOnlineStoreCreator passes "bacth_size": "1000" in its config dict, which fails MySQLOnlineStoreConfig validation with Extra inputs are not permitted.
-
The batch path serializes entity keys with entity_key_serialization_version=2, while the single-row write path and online_read in the same file use version 3. Rows written through the batch path are keyed differently from how they are read, so they cannot be read back. (Version < 3 is also discouraged per RepoConfig.)
Changes
mysql.py: bacth_size → batch_size; batch path entity_key_serialization_version 2 → 3 (matching the rest of the file).tests/universal/.../online_store/mysql.py: "bacth_size" → "batch_size" in BatchWriteMySQLOnlineStoreCreator.- New
tests/unit/infra/online_store/test_mysql_online_store.py: two mocked-connection unit tests (no live DB) — one asserting the batch path reads batch_size without crashing, one asserting it serializes entity keys with version 3. Both fail on master and pass with this change.
Testing
pytest sdk/python/tests/unit/infra/online_store/test_mysql_online_store.py sdk/python/tests/unit/infra/online_store/test_mysql_versioning.py → 13 passed.ruff check / ruff format --check clean; mypy feast/infra/online_stores/mysql_online_store/mysql.py clean.- No live MySQL / testcontainers run locally (no Docker in this environment).
Notes
Analysis and drafting were AI-assisted; I have reviewed and tested the change. Unlike postgres.py / sqlite.py, this store hardcodes the serialization version rather than honoring config.entity_key_serialization_version — left as-is to keep this change minimal; happy to follow up.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The `batch_write` branch of `MySQLOnlineStore.online_write_batch` had two
bugs that made it unusable:
- It read `config.online_store.bacth_size` (typo), which raises
`AttributeError` on the pydantic config, so enabling `batch_write`
crashed before writing anything.
- It serialized entity keys with `entity_key_serialization_version=2`,
while the single-row write path and `online_read` in the same file use
version 3. Rows written via the batch path therefore could not be read
back.
Fix the field name to `batch_size` and align the batch path on
serialization version 3. Add unit tests (mocked connection, no live
database) covering both.
Note for a follow-up: unlike `postgres.py` / `sqlite.py`, this store
hardcodes the serialization version instead of honoring
`config.entity_key_serialization_version`; left as-is here to keep the
change minimal.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Sht1BdriG5qHfkf5aF5gfc
Signed-off-by: Karth <[email protected]>
shuchu
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, let's wait and check the workflow running results.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
⚠️ Please install the  to ensure uploads and comments are reliably processed by Codecov.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.13%. Comparing base (5ad5592) to head (638b8c0).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## master #6802 +/- ##
==========================================
+ Coverage 47.08% 47.13% +0.04%
==========================================
Files 419 419
Lines 51878 51878
Branches 7525 7525
==========================================
+ Hits 24429 24453 +24
+ Misses 25700 25672 -28
- Partials 1749 1753 +4 ... and 1 file with indirect coverage changes
Continue to review full report in Codecov by Harness.
Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ad5592...638b8c0. Read the comment docs.
🚀 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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch has not been deployed
No deployments
Reviewers
shuchu
Awaiting requested review from shuchu
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
