Skip to content

feat(mol): add MOL type/function and PATTERN index#1797

Open
862103595 wants to merge 4 commits into
milvus-io:masterfrom
862103595:feat/mol-support
Open

feat(mol): add MOL type/function and PATTERN index#1797
862103595 wants to merge 4 commits into
milvus-io:masterfrom
862103595:feat/mol-support

Conversation

@862103595

@862103595 862103595 commented Apr 7, 2026

Copy link
Copy Markdown

related:

  • add DataType.Mol
  • add FunctionType.MOL_FINGERPRINT
  • add PATTERN index enum (v1/v2)
  • support mol_smiles_data serialization and SMILES validation

@sre-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 862103595
To complete the pull request process, please assign yhmo after the PR has been reviewed.
You can assign the PR to them by writing /assign @yhmo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot

Copy link
Copy Markdown

Welcome @862103595! It looks like this is your first PR to milvus-io/milvus-sdk-java 🎉

Signed-off-by: xiejh <862103595@qq.com>
Signed-off-by: xiejh <862103595@qq.com>
@liliu-z

liliu-z commented Apr 20, 2026

Copy link
Copy Markdown
Member

sdk-bulkwriter/src/main/java/io/milvus/bulkwriter/BulkWriter.java:1 The sdk-bulkwriter module is missing Mol type support entirely. verifyByDatatype in BulkWriter.java, schema construction in ParquetUtils.java, and the string serialization branch in ParquetFileWriter.java all need case Mol: branches added — without them, bulk import of Mol data will throw or silently fail.

@yhmo yhmo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one correctness/test-coverage gap in the new MOL support that should be fixed before merging.

@yhmo yhmo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

@yhmo yhmo Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants