feat: implement MOL data type insert support in Go layer - #46957
feat: implement MOL data type insert support in Go layer#46957862103595 wants to merge 4 commits into
Conversation
Summary of ChangesHello @862103595, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Milvus's Go layer by introducing comprehensive support for inserting Molecular (MOL) data. It establishes a complete data flow, from initial validation of SMILES strings in the proxy layer and their conversion to a more efficient pickle format, through to the robust handling and persistence of this data within the storage layer. This enables users to seamlessly store and manage molecular information, ensuring data integrity and efficient binlog serialization. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Invalid PR Title Format Detected Your PR submission does not adhere to our required standards. To ensure clarity and consistency, please meet the following criteria:
Required Title Structure: Where Example: Please review and update your PR to comply with these guidelines. |
862103595
changed the title
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
mergify
Bot
added
kind/feature
|
@862103595 Please associate the related issue to the body of your Pull Request. (eg. "issue: #") |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the MOL (Molecular) data type in the Go layer, enabling the insertion of SMILES strings. The changes are comprehensive, covering data validation, serialization, and binlog persistence. The implementation correctly follows the patterns established for other data types in the codebase. My review focuses on improving maintainability by addressing code duplication and enhancing error reporting for better user feedback. I've identified a few areas where refactoring could reduce redundancy and a specific case where error handling could be more precise.
| log.Warn("insert invalid MOL data!! Transform to pickle failed", | ||
| zap.Error(err), | ||
| zap.Int("index", index)) | ||
| return merr.WrapErrIoFailedReason(err.Error()) |
There was a problem hiding this comment.
The error returned here, merr.WrapErrIoFailedReason, suggests an I/O failure. However, a failure in common.ConvertSMILESToPickle is more likely due to an invalid SMILES string, which is a user input error. It would be more accurate to use merr.WrapErrParameterInvalid to provide a clearer error message to the user. This will improve error diagnostics.
| return merr.WrapErrIoFailedReason(err.Error()) | |
| return merr.WrapErrParameterInvalid("valid SMILES string", "invalid SMILES string", fmt.Sprintf("failed to convert SMILES to pickle at index %d: %v", index, err)) |
| case schemapb.DataType_Mol: | ||
| val, ok := data.([]byte) | ||
| if !ok { | ||
| return merr.WrapErrParameterInvalidMsg("incorrect data type") | ||
| } | ||
| isValid := true | ||
| if len(validData) > 1 { | ||
| return merr.WrapErrParameterInvalidMsg("wrong input length when add data to payload") | ||
| } | ||
| if len(validData) == 0 && w.nullable { | ||
| return merr.WrapErrParameterInvalidMsg("need pass valid_data when nullable==true") | ||
| } | ||
| if len(validData) == 1 { | ||
| if !w.nullable { | ||
| return merr.WrapErrParameterInvalidMsg("no need pass valid_data when nullable==false") | ||
| } | ||
| isValid = validData[0] | ||
| } | ||
| return w.AddOneMolToPayload(val, isValid) |
There was a problem hiding this comment.
This block of code for handling DataType_Mol is nearly identical to the existing case for DataType_Geometry. This duplication makes the code harder to maintain. Consider refactoring this shared logic into a helper function to reduce redundancy and improve readability.
| func mergeMolField(data *InsertData, fid FieldID, field *MolFieldData) { | ||
| if _, ok := data.Data[fid]; !ok { | ||
| fieldData := &MolFieldData{ | ||
| Data: nil, | ||
| ValidData: nil, | ||
| Nullable: field.Nullable, | ||
| } | ||
| data.Data[fid] = fieldData | ||
| } | ||
| fieldData := data.Data[fid].(*MolFieldData) | ||
| fieldData.Data = append(fieldData.Data, field.Data...) | ||
| fieldData.ValidData = append(fieldData.ValidData, field.ValidData...) | ||
| } |
There was a problem hiding this comment.
The function mergeMolField is almost a direct copy of mergeJSONField. Since MolFieldData and JSONFieldData have the same structure, this is a great opportunity to use generics to create a single mergeBytePayloadField function. This would eliminate code duplication and make the system easier to extend with similar data types in the future.
|
@862103595 go-sdk check failed, comment |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (76.96%) is below the target coverage (77.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #46957 +/- ##
==========================================
+ Coverage 74.36% 76.96% +2.60%
==========================================
Files 1462 2064 +602
Lines 241270 338815 +97545
==========================================
+ Hits 179423 260784 +81361
- Misses 53637 69804 +16167
- Partials 8210 8227 +17
🚀 New features to boost your workflow:
|
a6d0c0b to
f6dbae9
Compare
|
@862103595 go-sdk check failed, comment |
f6dbae9 to
4053377
Compare
|
@862103595 Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
mergify
Bot
added
needs-dco
4053377 to
cb9bd2a
Compare
|
@862103595 go-sdk check failed, comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 862103595 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
31d55d2 to
b3b63e3
Compare
|
@862103595 go-sdk check failed, comment |
b3b63e3 to
598bca2
Compare
sre-ci-robot
added
the
area/dependency
598bca2 to
3ebee18
Compare
sre-ci-robot
added
low-code-coverage
759149c to
b4bdc86
Compare
|
@862103595 Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
mergify
Bot
added
needs-dco
sre-ci-robot
removed
the
low-code-coverage
e7648cd to
6f46cde
Compare
mergify
Bot
added
dco-passed
sre-ci-robot
added
low-code-coverage
6f46cde to
02a2e66
Compare
|
rerun ut-cpp |
|
Hello 862103595, you are not in the organization, so you do not have the permission to rerun the workflow, please contact |
2d30cda to
06d3535
Compare
sre-ci-robot
added
low-code-coverage
06d3535 to
7974c46
Compare
|
@862103595 go-sdk check failed, comment |
|
rerun go-sdk |
- Add MOL data type insert support with SMILES validation - Implement SMILES↔Pickle conversion for storage and query return path - Add FillWithNullValue and FillWithDefaultValue support for MOL type - Sync milvus-proto for MOL data type definition - Add unit tests with 80%+ coverage for MOL basic functionality Signed-off-by: xiejh <862103595@qq.com>
Add genEmptyMolFieldData to handle empty MOL field data generation, fixing "unsupported data type: Mol" error when query returns empty results with MOL output fields. Signed-off-by: xiejh <862103595@qq.com>
Remove stale milvus-proto version entries from go.sum and pkg/go.sum, and fix go-commons-pool position in go.mod to match upstream. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xiejh <862103595@qq.com>
Add tests for AppendFieldDataByColumn, GenEmptyFieldData, payload writer/reader error paths, and print_binlog MOL branch to reach 80% patch coverage requirement. Signed-off-by: xiejh <862103595@qq.com>
3 participants
Related to
Design Doc
This PR is part of the MOL feature stack and should be reviewed together with the design doc above.
Summary
This PR implements the insert path support for MOL (Molecular) data type in the Go layer of Milvus. It enables users to insert molecular data (SMILES strings) into collections, with proper validation, serialization, and binlog persistence.
Changes
1. Proxy Layer - Data Validation (
internal/proxy/validate_util.go)checkMOLFieldData()function to validate MOL field data2. Storage Layer - Data Structures and Operations
Core Data Structure (
internal/storage/insert_data.go)MolFieldDatastruct to represent MOL field dataFieldDatainterface methods:RowNum(),GetRow(),GetDataRows()AppendRow(),AppendRows(),AppendDataRows()AppendValidDataRows(),GetDataType()GetMemorySize(),GetRowSize(),GetNullable()Serialization (
internal/storage/data_codec.go)AddFieldDataToPayload()for binlog serializationAddInsertData()for data insertionPayload Operations
Writer (
internal/storage/payload_writer.go):AddOneMolToPayload()method to write MOL data to payloadAddDataToPayloadForUT()MilvusDataTypeToArrowType()Reader (
internal/storage/payload_reader.go):GetMolFromPayload()method to read MOL data from payloadGetDataFromPayload()Data Conversion (
internal/storage/utils.go)ColumnBasedInsertMsgToInsertData()- converts proto FieldData to MolFieldDataTransferInsertDataToInsertRecord()- converts MolFieldData back to proto FieldDatamergeMolField()function for merging MOL field dataOther Utilities
internal/storage/data_sorter.go): Added MOL type support for data sortinginternal/storage/print_binlog.go): Added MOL type support for binlog debugginginternal/storage/serde.go): Added MOL type supportinternal/storage/payload.go): Added MOL methods to interfacesImplementation Details
Data Flow
MolFieldDatastructure from protoFieldDataKey Features
Files Changed
internal/proxy/validate_util.go(+37 lines)internal/storage/data_codec.go(+22 lines)internal/storage/data_sorter.go(+3 lines)internal/storage/insert_data.go(+5 lines)internal/storage/payload.go(+2 lines)internal/storage/payload_reader.go(+22 lines)internal/storage/payload_writer.go(+44 lines)internal/storage/print_binlog.go(+16 lines)internal/storage/serde.go(+1 line)internal/storage/utils.go(+40 lines)Total: 10 files changed, 192 insertions(+)
Notes