feat(mol): add MOL type/function and PATTERN index#1797
Conversation
|
[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 |
|
Welcome @862103595! It looks like this is your first PR to milvus-io/milvus-sdk-java 🎉 |
019680b to
1744f10
Compare
Signed-off-by: xiejh <862103595@qq.com>
Signed-off-by: xiejh <862103595@qq.com>
1744f10 to
4c84d4f
Compare
|
sdk-bulkwriter/src/main/java/io/milvus/bulkwriter/BulkWriter.java:1 The sdk-bulkwriter module is missing Mol type support entirely. |
yhmo
left a comment
There was a problem hiding this comment.
Found one correctness/test-coverage gap in the new MOL support that should be fixed before merging.
yhmo
left a comment
There was a problem hiding this comment.
Adding the inline note that failed to attach in the earlier review.
| molBuilder.addData("C" + i); | ||
| } | ||
| testScalarField(ScalarField.newBuilder().setMolSmilesData(molBuilder).build(), | ||
| DataType.Mol, dim); |
There was a problem hiding this comment.
The added test only covers FieldDataWrapper readback for mol_smiles_data. This PR also changes the write-side conversion paths in ParamUtils (checkFieldData, checkFieldValue, genScalarField, objectToValueField, valueFieldToObject) plus the V2 enum mapping for DataType.Mol(27), but none of those new paths are exercised here. Since MOL support is mostly serialization/deserialization plumbing, it would be safer to add at least one request-side test that inserts/upserts a MOL scalar value and one enum mapping assertion for code 27, otherwise a regression on the write path could still pass these tests.
related:
DataType.MolFunctionType.MOL_FINGERPRINTPATTERNindex enum (v1/v2)mol_smiles_dataserialization and SMILES validation