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

Compound Wildcard Indexes #4790

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Compound Wildcard Indexes #4790

wants to merge 5 commits into from

Conversation

marcingrzejszczak
Copy link
Contributor

No description provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 16, 2024
@marcingrzejszczak
Copy link
Contributor Author

Question about the current implementation

A ccording to the tests and the implementation the following combinations are NOT allowed (a MappingException is being thrown)

  • wildcardFieldName NOT empty and wildcardProjection NOT empty
  • wildcardFieldName EMPTY and wildecardProjection EMPTY

While the first condition is correct I have doubts about the second one.

If wildcardFieldName is empty, that implies the default $** value. That in effect means that all fields are scanned and wildcardProjection is optional. Why is an exception being thrown?

cc @christophstrobl

@christophstrobl
Copy link
Member

Yeah, you can create the index using an all fields term without narrowing down the scope via projections.
Would it make sense to switch the default value for wildcardFieldName to $** and replace the empty checks with something like an isAllFieldsIndexTerm(...) so we can get the code path there more expressive?

Another thing that crossed my mind is that you can specify more than one wildcard index on a collection - I guess same is true for compound wildcard ones, which would require the annotation to be repeatable.

@marcingrzejszczak
Copy link
Contributor Author

I looked into the code and re:

Would it make sense to switch the default value for wildcardFieldName to $** and replace the empty checks with something like an isAllFieldsIndexTerm(...) so we can get the code path there more expressive?

If we look at @CompoundIndex it has the def method

String def() default "";

and the Javadoc states

  • If left empty on nested document, the whole document will be indexed.

Which means that if we go with the approach that the wildcardFieldName would be equal to $** then I think we would be inconsistent. WDYT?

@christophstrobl
Copy link
Member

I do not agree - we've got the fields attribute that translates to the def of the compound index. So if that is blank and the default for wildcardFieldName is $** the entire doc would be indexed, right?

@marcingrzejszczak
Copy link
Contributor Author

OK I understand. For future reference we're talking about @CompoundWildcardIndexed that has fields method that delegates to @CompoundIndex's def method. Now if @CWI is empty which is the default that would be that @CI def is empty which means that it translates to $** which indexes the whole doc.

@marcingrzejszczak marcingrzejszczak marked this pull request as ready for review September 25, 2024 09:40
@christophstrobl
Copy link
Member

Does it make sense to provide a CompoundWildcardIndexes container to allow repeatable usage? Would also be good to have a test that is using value expressions for the index name to see if resolving those work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants