-
Notifications
You must be signed in to change notification settings - Fork 2
[core] add metaxy_deleted_at column #502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Coverage Report (Python 3.13) •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (Python 3.12) •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Test Results (Python 3.10)1 969 tests 1 947 ✅ 6m 33s ⏱️ Results for commit ab1b584. ♻️ This comment has been updated with latest results. |
Coverage Report (Python 3.10) •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
a5d005a to
f892f10
Compare
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.
Pull request overview
This PR implements soft-delete functionality for metadata rows, allowing rows to be marked as deleted via a timestamp without physical removal from the store.
- Adds a new
metaxy_deleted_atcolumn that defaults to NULL for active rows - By default, soft-deleted rows are filtered out in
read_metadata()calls - Adds an
include_deletedparameter to opt-in to viewing deleted rows - Ensures backward compatibility with legacy data that lacks the new column
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/metaxy/models/constants.py |
Adds METAXY_DELETED_AT constant and includes it in system column sets |
src/metaxy/metadata_store/base.py |
Implements soft-delete filtering logic in read_metadata() and adds default NULL value in _add_system_columns() |
src/metaxy/versioning/engine.py |
Adds METAXY_DELETED_AT to allowed system columns for upstream joins and moves imports to top |
src/metaxy/metadata_store/ibis.py |
Adds .lazy() call to ensure consistent LazyFrame behavior with Narwhals |
tests/metadata_stores/test_provenance_golden_reference.py |
Adds comprehensive test for soft-delete behavior with tombstone records |
src/metaxy/_testing/parametric/metadata.py |
Updates test helpers to include metaxy_deleted_at column with NULL default |
tests/ext/__snapshots__/test_sqlmodel.ambr |
Updates snapshots to include the new metaxy_deleted_at field in all records |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danielgafni
left a comment
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.
Looking good overall! Just small comments
f892f10 to
5aed737
Compare
danielgafni
left a comment
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.
LGTM!
|
should we already merge this or get the whole stack ready first? |
danielgafni
left a comment
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.
Actually @geoHeil the sqlalchemy and sqlmodel plugins and tests have to be updated as well
ca79e46 to
1a920c5
Compare
added - tests were green. would we need any additional tests or is that good? |
16e3d71 to
1a9d523
Compare
|
wrong PR sorry |
1a9d523 to
22465e0
Compare
danielgafni
left a comment
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.
Ok cool, let's merge it a little later as part of the whole stack - I don't want to redo my migrations for Anam's prod right now
b29bc0b to
01ee00e
Compare
src/metaxy/metadata_store/base.py
Outdated
| allow_fallback: If `True`, check fallback stores on local miss | ||
| current_only: If `True`, only return rows with current feature_version | ||
| latest_only: Whether to deduplicate samples within `id_columns` groups ordered by `metaxy_created_at`. | ||
| include_soft_deleted: If `True`, include soft-deleted rows in the result. |
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.
I think it's worth noting that previous historical materializations of the same feature version will be effectively removed from the output as well.
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.
You mean like:
include_soft_deleted: If `True`, include soft-deleted rows in the result. Previous historical materializations of the same feature version will be effectively removed from the output otherwise.
01ee00e to
da0b24b
Compare
a308098 to
5e9126b
Compare
5e9126b to
ab1b584
Compare

resolves: #245