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

DATAMONGO-1467 - Support partial filter expressions in indexing. #380

Closed
wants to merge 1 commit into from

Conversation

d0x
Copy link

@d0x d0x commented Jul 26, 2016

Implementing the partialFilterExpression.

Please have a look to the following points in detail:

  • On the @Indexed-Annotation I used the name partialFilter instead of mongos name partialFilterExpression. I thought the word "expression" may leads into wrong assumptions regarding Springs Expression language
  • I'm not able to test the change in action because with the current branch I get the following Error Error:(5, 8) java: cannot access org.springframework.data.repository.query.QueryByExampleExecutor class file for org.springframework.data.repository.query.QueryByExampleExecutor not found

@odrotbohm
Copy link
Member

Thanks, Christian. This looks decent. I wonder whether the API on Index should rather take a Criteria instance to prevent users from having to deal with strings when using the API directly.

I am still a bit torn on the annotation attribute as it feels like that might take us a bit to far into programming in annotations. Granted, there is @Query that already takes a MongoDB query, but I tend to be more conservative when it comes to annotations used on domain types. Another thing that plays into this is that I don't even think we'd introduce indexing annotations in the first place if we started from scratch as they turn out to be quite problematic due to MongoDB's automatic collection creation behavior (when you drop a collection and save a new object, the indexes are not recreated as MongoDB recreates the collection behind your back).

So I wonder: would you be terribly disappointed if we dropped the annotation attribute and required users to manually create (more complicated) indexes through the API? I feel like this would hint users to a model where they get more control over when indexes are created and could also trigger index recreation explicitly whenever they want.

@odrotbohm odrotbohm changed the title DATAMONGO-1467 - Support of partialFilterExpression DATAMONGO-1467 - Support partial filter expressions in indexing. Jul 26, 2016
@d0x
Copy link
Author

d0x commented Jul 26, 2016

Hey Oliver, of course I would be disappointed, as you know, programmers need appreciation ;). But I understand your argumentation. And it sounds valid. Since you told me on the spring io ?2014/15?, that you don't really like the @Indexed annotations because it implies simplicity where there isn't. Anyway, yesterday I thought this is a smart move. :)

So its up to you. If you like to have some changes, - tell me, otherwise feel free to close it. And I will not be terribly disappointed, - only dissappointed :)

Best Regards,
Christian.

@odrotbohm
Copy link
Member

Okay, what about the following plan: we start with the implementation in the Index API in the first place, resolve, DATAMONGO-1467 and immediately create a ticket to suggest adding the annotation attribute explaining why we didn't add it in the first place to have a dedicated spot to gather feedback. Adding the attribute in a later milestone or release candidate is not a hard thing to do assuming we get convincing arguments.

Do you want me to take over regarding that change and the one regarding Criteria on Index? Or do you want to go ahead? We have a milestone release scheduled for late tomorrow / early Thursday which we could try to sneak this in if we act quickly.

@d0x
Copy link
Author

d0x commented Jul 26, 2016

That sounds like a plan. The problem with the schedule is that I can work only a view hours on the train today and tomorrow. And I would need to get familiar with the Criteria-API first to make this change. I could do it after friday.

I could offer that I remove the @Indexed annotation stuff today afternoon. After that I would try to do it with the Criteria-API. (but the Index IndexInfo would still return a String, right?

I would also have no problem if you take over. To be honest I think till early Thursday I have to less time for it.

@odrotbohm
Copy link
Member

Never mind, I can just take it. Just didn't want to step on your toes :).

@odrotbohm odrotbohm self-assigned this Jul 26, 2016
@d0x
Copy link
Author

d0x commented Jul 26, 2016

Thank you very much :).

@odrotbohm
Copy link
Member

Superseded by #431.

@odrotbohm odrotbohm closed this Jan 30, 2018
@christianfinckler1212
Copy link

I think, it is sad, to not implement that as an annotation. Now I have to specify my indexes at different places or do not use annotations at all anymore

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.

None yet

3 participants