Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HSEARCH-3909 Allow looking up the capabilities of each field in the metamodel #3865

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Dec 12, 2023

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

Best reviewed commit by commit, as the first commit is essentially a refactoring of tests to allow for easier testing of this new feature. (you'll probably want to skip reviewing that)

See the changes to the documentation and to SearchMappingIndexedEntitiesIT for a (short) description of the feature.

Note I chose to use strings for traits in anticipation for https://hibernate.atlassian.net/browse/HSEARCH-3633: if we want to be able to use traits in annotations, we can't use a custom type, and enums aren't great either as they aren't extensible by the Lucene/Elasticsearch backends.

@yrodiere yrodiere force-pushed the HSEARCH-3909 branch 4 times, most recently from 30bf152 to 0339714 Compare December 12, 2023 15:32
@yrodiere
Copy link
Member Author

What the hell is this: https://sonarcloud.io/organizations/hibernate/rules?open=java%3AS5841&rule_key=java%3AS5841
I don't know if the trait sets are empty, and I don't care, I just want to check that they don't contain a particular trait :(
I'll see if I can alter the Sonar profiles.

Copy link
Member

@marko-bekhta marko-bekhta left a comment

Choose a reason for hiding this comment

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

nice 👍🏻
I've noticed that in tests we aren't always using trait constants but just strings. But I can't say that there's much benefit in going through all the tests to find them 😃.
I was wondering about the identifier field: is it needed for Infinispan?

@yrodiere
Copy link
Member Author

I've noticed that in tests we aren't always using trait constants but just strings.

Yes that's on purpose, as this makes sure that tests will fail if we change constants (that's an API change, shouldn't happen).

We (generally) do the same thing in tests of configuration properties.

But I can't say that there's much benefit in going through all the tests to find them 😃.

Agreed :)

I was wondering about the identifier field: is it needed for Infinispan?

The identifier is not a field... Not sure what you're referring to?

AFAIK we only allow the id predicate and id projection on identifers, and we always allow them, so the "traits" for the identifier would never change... doesn't seem useful?

Copy link

@marko-bekhta
Copy link
Member

The identifier is not a field... Not sure what you're referring to?

AFAIK we only allow the id predicate and id projection on identifers, and we always allow them, so the "traits" for the identifier would never change... doesn't seem useful?

Yes, I'm just not sure how Infinispan is going to use these traits and if they are treating an id as a field or not using the id at all in their queries, so that was why I've asked if we need to somehow include the id into all this 😃

@yrodiere
Copy link
Member Author

The identifier is not a field... Not sure what you're referring to?
AFAIK we only allow the id predicate and id projection on identifers, and we always allow them, so the "traits" for the identifier would never change... doesn't seem useful?

Yes, I'm just not sure how Infinispan is going to use these traits and if they are treating an id as a field or not using the id at all in their queries, so that was why I've asked if we need to somehow include the id into all this 😃

Okay, well as I said the capabilities of the identifier don't change, so they can hardcode it. And if they index the id as a field, they'll have metadata for that field. So, all good until we get feedback hinting otherwise 🤷

I'm not sure keys are indexed in Infinispan at the moment, though. That was a planned feature at some point, no idea if it was implemented.

@yrodiere
Copy link
Member Author

Hey @fax4ever , here's the PR to allow you to look up field capabilities ("traits"), in case you want to have a look. This will allow you to determine if the knn predicate can be used, among other things.

@fax4ever
Copy link
Contributor

Hey @fax4ever , here's the PR to allow you to look up field capabilities ("traits"), in case you want to have a look. This will allow you to determine if the knn predicate can be used, among other things.

Thank you. We're going to use it to validate the query instance

@yrodiere yrodiere merged commit 91a2df2 into hibernate:main Dec 14, 2023
10 checks passed
@yrodiere yrodiere deleted the HSEARCH-3909 branch January 29, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants