HSEARCH-5021 Improve support for custom value bridges for vector fields

[marko-bekhta]

https://hibernate.atlassian.net/browse/HSEARCH-5021

This is just a draft for now so we can talk over the code 😃

[yrodiere]

Hey @marko-bekhta is this ready for review?

[marko-bekhta]

hmm, good question 😃, looking at the changes, I don't see something to add...

[yrodiere]

From https://hibernate.atlassian.net/browse/HSEARCH-5021:

At the moment (as of https://hibernate.atlassian.net/browse/HSEARCH-4948 ), if someone was to write a custom binder for vector fields with a custom field type:

1. They need to know about the number of dimensions, because that’s a mandatory parameter of IndexFieldTypeFactory#asVector… but they don't have access to the number passed to @VectorField#dimension.
2. If they want to force the number of dimensions (because their bridge always produces vectors with a certain number of dimensions), users of the binder will still need to pass something to @VectorField#dimension in the mapping. Even though that value will never be used…

Does this PR addresses any of that? I'm honestly not sure :x

Perhaps it would be best to add/find an integration test for each of those:

1. A custom bridge that turns for example a List<Float> into a float[], and declares the field type explicitly as projectable by default, but doesn't know about the number of dimensions. Users must provide the number of dimensions in @VectorField.
2. A custom bridge that turns for example a Triplet<Float, Float, Float> into a float[], and declares the field type explicitly with dimensions=3. Users must not be forced to provide a number of dimensions in @VectorField.

I think org.hibernate.search.integrationtest.mapper.pojo.mapping.definition.VectorFieldIT#customBridge_explicitFieldType already addresses the second item.

I can't find something about the first, though... Which I guess makes sense, since org.hibernate.search.engine.backend.types.dsl.IndexFieldTypeFactory#asVector forces you to pass a number of dimensions ATM?

[marko-bekhta]

I can't find something about the first, though... Which I guess makes sense, since org.hibernate.search.engine.backend.types.dsl.IndexFieldTypeFactory#asVector forces you to pass a number of dimensions ATM?

Right. And the initial idea was to add annotation attributes (@VectorField ones in particular) to the parameters so that the user would have access to dimension value from the annotation and could pass it to asVector call ... but then you were saying that that would be a users-writes-their-own-annotation-and-processor case so I assumed that that covers it and nothing to be done about it 😄...

Now if we want to allow calls like asVector() without an initial dimension value ... I suppose we can do that, but that would mean reverting the changes we've done in the other PR... let me put something together and we can check it tomorrow.

[hibernate-github-bot]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[yrodiere]

Now if we want to allow calls like asVector() without an initial dimension value ... I suppose we can do that

Thanks, looks like this addresses the issue completely! A few comments below.

but that would mean reverting the changes we've done in the other PR...

Yeah sorry about that. I guess I failed to consider this use case when I suggested that change in the initial PR...

the initial idea was to add annotation attributes (@Vectorfield ones in particular) to the parameters so that the user would have access to dimension value from the annotation and could pass it to asVector call ...

Right, and that created a few problems.

but then you were saying that that would be a users-writes-their-own-annotation-and-processor case so I assumed that that covers it and nothing to be done about it 😄...

Yeah I was just talking about the specific case where the binder needs to get the number of dimensions as a parameter. Not about cases where the binder doesn't need to know about the number of dimensions (because it's implied by input).

[marko-bekhta]

yeah, no worries 😃 and sorry that I didn't catch it that we should make these changes from your initial review 🙈 😃. I've applied your suggestions and cleaned up PropertyMappingVectorFieldOptionsStepImpl a bit more.

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
95.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[yrodiere]

LGTM, thanks!