enhance: use internal API for Loon FFI writer to fix properties nullptr issue #45871
+377
−74
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tr issue This PR switches the Loon FFI packed writer from the high-level FFI interface to the lower-level internal C++ API to address a critical bug where properties keys become nullptr when writing large amounts of data. ***Problem*** When using the original FFI interface (`writer_new`, `writer_write`, `writer_close`, `transaction_begin`, `transaction_commit`) for writing large datasets, the properties map keys would become nullptr after extended operations. This manifests as: - Segmentation faults during write operations - Corrupted manifest data - Unpredictable behavior during transaction commits The root cause appears to be related to memory management in the FFI boundary layer when handling long-lived properties objects across multiple FFI calls. ***Solution*** Replace the FFI-based writer implementation with direct calls to the internal milvus-storage C++ API: - Use `milvus_storage::api::Writer` directly instead of FFI wrapper - Use `milvus_storage::api::transaction::TransactionImpl` for commits - Construct `arrow::RecordBatch` directly from imported arrays This bypasses the problematic FFI properties handling while maintaining the same functionality. Also bump milvus-storage version resolving get chunk bug. Related to milvus-io#44956 Signed-off-by: Congqi Xia <[email protected]>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mergify
bot
added
dco-passed
Contributor
|
[ci-v2-notice]
To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
Contributor
|
@congqixia go-sdk check failed, comment |
Signed-off-by: Congqi Xia <[email protected]>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (0.51%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #45871 +/- ##
==========================================
- Coverage 76.15% 76.11% -0.05%
==========================================
Files 1881 1870 -11
Lines 294229 292469 -1760
==========================================
- Hits 224082 222603 -1479
+ Misses 62701 62489 -212
+ Partials 7446 7377 -69
🚀 New features to boost your workflow:
|
Contributor
Author
|
/ci-rerun-ut-integration |
1 similar comment
Contributor
Author
|
/ci-rerun-ut-integration |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR switches the Loon FFI packed writer from the high-level FFI interface to the lower-level internal C++ API to address a critical bug where properties keys become nullptr when writing large amounts of data.
Problem
When using the original FFI interface (
writer_new,writer_write,writer_close,transaction_begin,transaction_commit) for writing large datasets, the properties map keys would become nullptr after extended operations. This manifests as:The root cause appears to be related to memory management in the FFI boundary layer when handling long-lived properties objects across multiple FFI calls.
Solution
Replace the FFI-based writer implementation with direct calls to the internal milvus-storage C++ API:
milvus_storage::api::Writerdirectly instead of FFI wrappermilvus_storage::api::transaction::TransactionImplfor commitsarrow::RecordBatchdirectly from imported arraysThis bypasses the problematic FFI properties handling while maintaining the same functionality.
Also bump milvus-storage version resolving get chunk bug.
Related to #44956