-
Notifications
You must be signed in to change notification settings - Fork 9
Fix export part crash and add docs for export partition #1161
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
Conversation
|
@codex review the changes in export part |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .metadata_version = metadata_snapshot->getMetadataVersion(), | ||
| .min_part_metadata_version = manifest.data_part->getMetadataVersion() | ||
| }; | ||
|
|
||
| auto mutations_snapshot = storage.getMutationsSnapshot(mutations_snapshot_params); |
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.
Preserve query mutation settings when exporting parts
The export task now reconstructs the mutations snapshot with only metadata_version and min_part_metadata_version, relying on the storage context defaults. User settings such as apply_mutations_on_fly (needed to apply unfinished mutations or lightweight deletes during export) are no longer propagated from the original EXPORT PART query, so getMutationsSnapshot returns an empty data/alter mutation set and the exported part can miss pending mutations even when the query explicitly requested them. Previously the snapshot carried the query-context settings via storage_snapshot, so this is a regression for exports that expect on-the-fly mutation application.
Useful? React with 👍 / 👎.
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .metadata_version = metadata_snapshot->getMetadataVersion(), | ||
| .min_part_metadata_version = manifest.data_part->getMetadataVersion() | ||
| }; | ||
|
|
||
| auto mutations_snapshot = storage.getMutationsSnapshot(mutations_snapshot_params); |
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.
Keep data/alter mutations in export part snapshot
The new mutations snapshot built in executeStep only sets metadata_version and min_part_metadata_version, leaving need_data_mutations, need_alter_mutations, and patch parameters at their defaults. getMutationsSnapshot therefore returns an empty snapshot whenever metadata versions match, so DELETE/UPDATE mutations or patch parts in progress are no longer applied during export. Compared to the previous use of manifest.storage_snapshot->data->mutations_snapshot (which honored apply_mutations_on_fly/apply_patch_parts), exporting a part while mutations are pending will emit stale data that does not match normal query results.
Useful? React with 👍 / 👎.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix crash due to circular dependency
Documentation entry for user-facing changes
CI/CD Options
Exclude tests:
Regression jobs to run: