[HSEARCH-886] Provide the ability to configure specific paths to index within @IndexEmbedded as an alternative to depth

[DavideD]

A first prototype.

Maybe when the paths is set depth should have 0 has default value.

Let me know what you think.

Davide

[DavideD]

I've added the validation of the paths and set default depth to 0 if you only set the paths attribute.

[DavideD]

I've removed the validation. I need to think a bit more about it

[Sanne]

Hi Davide,
I had a brief look and I'm liking what I'm seeing. Liking it a lot.

While playing with this, can you consider as well HSEARCH-638 ? It seems you're already working on the depth, adding it as a parameter.

What's the validation you removed?

I think you should add more tests, especially some to mix the different strategies (to have both depth and paths)

[DavideD]

I'm very happy you like it.

Zach Kurey tried it and he gave me some suggestions:
https://hibernate.onjira.com/browse/HSEARCH-886

Since the paths are specified as a String sometimes you can have some errors in it (for example when you mispelled the name of a field). It would be nice to have some kind of validation for this cases. I've pushed a first version that throws an exception when a path doesn't exists but it doesn't seem towork in every case at the moment so I prefered to remove it.

I think you are right about adding tests...it was actually a bit of lazyness... :)

[Sanne]

Right, adding validation on the attribute names makes sense. Remember it should work on both getters and field accessors.

[Sanne]

BTW, keep rebasing it as we're doing many changes ;)

[DavideD]

No problem ;)

[DavideD]

I've added the test for the depth and rebased the project to the latest master version.
I have one problem at the moment: if I run the tests I've added in eclipse everything works fine but if I execute the maven build they fail.

My maven build doesn't look very reliable lately. Can anyone try it?

Thanks

[Sanne]

tried it, only your tests fail:

`Tests in error:
testPathIsIndexed(org.hibernate.search.test.embedded.path.PathEmbeddedTest): Unknown entity: org.hibernate.search.test.embedded.path.C
testPathIsIndexedWithPrefix(org.hibernate.search.test.embedded.path.PathEmbeddedTest): Unknown entity: org.hibernate.search.test.embedded.path.C
testMultiFieldsAreIndexedIfInPath(org.hibernate.search.test.embedded.path.PathEmbeddedTest): Unknown entity: org.hibernate.search.test.embedded.path.C
testEmbeddedNotIndexedIfNotInPath(org.hibernate.search.test.embedded.path.PathEmbeddedTest): Unknown entity: org.hibernate.search.test.embedded.path.C
testFieldNotIndexedIfNotInPath(org.hibernate.search.test.embedded.path.PathEmbeddedTest): Unknown entity: org.hibernate.search.test.embedded.path.C

Tests run: 392, Failures: 0, Errors: 5, Skipped: 0
`

[DavideD]

Thanks,
they seem sto work in eclipse...

I will take another look this evening.

[DavideD]

It seems the problem was in the setup and teardown methods.
Now everything seems to work.

Don't know if it comes in handy but after the latest rebase the CollectionUpdateEventTest doesn't fail anymore.

[Sanne]

CollectionUpdateEventTest is not failing anymore because I've "silenced" it with the following change:
50b7b1e

but there still is a problem with it, Hardy is looking into it. at least it's not annoying people until we find out.

I just checked the tests of your last version, and all is green for me too. But I'm too sleepy to inspect the code now :)

@zackurey could you check this out too and see if it's going to solve your needs?
If not, or if you have any doubt, could you please help Davide with additional tests, especially thinking about the tricky corner cases?

[Sanne]

And congratulations for surviving the GIT skills tests after all the code reorganization we did in the last few days ;)

[zackurey]

@Sanne
Definitely. I've been trying to get back around to looking at the latest changes today, but am running low on time. I'll give it it a review and a spin soon(hopefully tomorrow).

Thanks Davide.

[DavideD]

The validation of the path is still missing though. If everything else works maybe it can become a new JIRA.

@Sanne
Git did most of the work :)

Cheers

[zackurey]

@davide

I'd be happy to take a crack at the path validation work. Seems only fair since I'm pushing for the feature. I'm not sure if we'd want it to come in as a different JIRA or not. I'm fine with it if Sanne is. If I'm having to fork your fork, things could get convoluted coordinating things.

Let me take a look over the latest and see if there is anything that needs changing with the basic feature support. The last time I gave it a spin everything worked fine.

[Sanne]

you can work as you think is best, we can include Davide's work first and add validation later, or Zack can start working on validation from Davide's work.

I don't think Davide will need to rebase as often as in the last few days as we definitely don't move around files every week, but make sure you talk to each other so that he stops rebasing when you fork from this branch, or it will be hard to keep in sync.
@DavideD can you stop rebasing? do you think it's stable enough that you can apply eventually needed improvements/fixes on top of it as additional commits?

As far as timing goes, I'd be happy to include Davide's patch as soon as possible (even without the validation) but since it's an important change I'd love it if @zackurey could test it first to see if all was clear on the requirements, if you can now practically express the minimum graph needed for your real world use case (likely more complex than the unit tests). That's what I meant with helping with testing, you might have some advanced test case you want to add.

Generally it's a great idea to add your own product's use case as a test, so you'll know for sure the used frameworks won't break that in future ;)

[zackurey]

I think I'd prefer to work on it separately, just to reduce the number of hoops to jump through.

@real world use case
Actually I already integrated Davide's changes into a custom 3.4.1 hibernate search lib and used it in our app. The performance gains for the problem @IndexEmbedded marked entity were huge. 5x faster huge.

I used it in two locations that were performance problems. Each with > 5 paths per @IndexEmbedded. A variety of different use cases are covered by each path.

• Leaf ending in a field bridge
• Leaf ending in simple types
• Leaf ending in a field bridge for a collection of simple types(Longs)
• Paths where a portion of the path in the middle is a @IndexEmbedded Collection

I don't really have a use case right now for a mixed bag case where we have a default depth + some paths that are deeper than the depth. But I'll contrive one up.

Also all the other traditional depth based indexing done in the app was unharmed, and worked as it always has.

I commented on a particular bit of code that could potentially mis-identify a path match. But as long as that is fixed I think its probably good to go. I should re-merge/integrate into a 3.4.1 branch and test again though. I tried upgrading to Hibernate core and decided to abandon the attempt for now. Nothing wrong with 4.x, just a few changed apis that we used a lot for custom type conversion, and some deprecated annotations/apis that I'll have to verify the substitutes for actually work as intended.

[Sanne]

@zackurey
thanks a lot for the prompt feedback. Just to clarify there are no misunderstandings, I don't think we will include this in 3.4, you'll have to maintain your own branch if you don't upgrade Core ..

BTW the "at" symbol has a real meaning on github, make sure you use it to ping the correct nicknames. So far you notified mister "real" and "davide" already.

@DavideD, do you have a test to protect the code from the mis-identification Zack just mentioned?

[zackurey]

thanks a lot for the prompt feedback. Just to clarify there are no misunderstandings, I don't think we will include this in 3.4, you'll have to maintain your own branch if you don't upgrade Core ..

Understood. 4.x rocks the boat to much for a near term release we have. Next release I'll take the necessary 4.x steps.

BTW the "at" symbol has a real meaning on github, make sure you use it to ping the correct nicknames. So far you notified mister "real" and "davide" already.

How embarrassing.. Thanks for limiting future mistakes. I frequently use 'at' as 'an answer' to something as well.

do you have a test to protect the code from the mis-identification Zack just mentioned?

I just reviewed the code. The suggested fix has been applied.

[DavideD]

I've applied all the fix suggested by @zackurey. I added the test to check that the depth parameter is respected (using the genealogy tree of John England). I didn't add the tests for the mis-identification (shame on me).

I don't have any problem in zack working on the validation. This solution seems stable enough and as long as no other bugs are found I won't touch it. At the moment I would just add a test for the mis-identification problem.
I think it's safe for zack to work on the validation starting from my work anyway.

[zackurey]

@DavideD
I re-tested your latest code in my app Davide. Everything worked as expected. I thought I saw a problem with a super classes @IndexedEmbedded collections not being picked up even though they were part of a path, but the issue has mysteriously gone away. I'll add a test for that and all of our other 'path' use cases as Sanne suggested above when I do the validation work.

@Sanne
Can you accept Davide's pull requests now? Looks pretty solid to me, and working from my own fork seems like the easiest way to proceed.

[zackurey]

@Sanne
Sorry to flip flop, but nevermind on pulling Davide's code in first. I just finished the validation + test cases. I'll fork Davide's fork first thing tomorrow and send the pull requests to him so you can pull everything in in one go. I'm out of energy for tonight.

[zackurey]

OK. Apparently I can't have two separate forks that share the same parent. When I try to fork Davide's fork nothing happens and it just drops me on the same page as my fork of hibernate-search.

I would delete my other repo/fork and then refork from Davide's but I'm waiting for my pull requests for HSEARCH-926 to be accepted. @Sanne, the contributor agreement is signed for both the corporate and individual level. Can that pull request proceed now?

The other option is @DavideD's work can get pulled into the main branch, and then I can pull the changes into my own fork.

If I'm missing something as a newb to Git, please point it out. Thx.

[Sanne]

Hi, sure I'll try to merge this soon, sorry have been busy with other issues.
TBH I still have to review this, so I don't know yet if some changes are required.

To answer your git doubts: yes you don't need to fork again. You should have now a copy of yourself on github, and a personal copy on your dev-station. Usually you push/pull to your github repository (conventionally named "origin") but you can add more "remote"s (remote is the git term), so you can pull changes from Davide, push them to your own, or pull changes from "upstream" (conventional name for the project reference). I will for example pull from both of you, test locally, send a proposal to my personal github to have that integrated from another committer if we feel more reviews are needed (as usually when new APIs are involved).
If you join us on IRC #hibernate-dev I can help you directly in the first steps.

[zackurey]

Thanks Sanne. Your nudge about the remote repos pointed me in the right direction. Neat stuff.

I've sent the pull request to Davide: DavideD#1. @DavideD, can you pull that in and let me know if you have any concerns, or thoughts about additional tests?

I could also send a pull request directly to hibernate search:master, which would contain both our changes. But just in case Davide needs to make further changes I went that route first. But if the other way is preferred let me know.

[DavideD]

Sorry @zackurey if I didn't reply to your messages in these days but I was on holiday. After a quick look everything seems great.

I will include it in the already existing pull request tomorrow when I'm less tired.

Cheers

[DavideD]

Rebased, cleaned and joined with the validation made by @zackurey.

Should be ready for a review.

Cheers

[Sanne]

Hi, sorry I didn't integrate it yet as I'm adding some tests; I'm not sure about the chosen algorithm being respecting the depth in both directions (i.e. from @ContainedIn too). Maybe my tests will prove me wrong but I'm not done with it yet.

The general code quality is very good, thanks! Some more comments could help, don't take the original source as an example of well commented code.

[DavideD]

It's my fault. I tend to avoid comments because most of the time they are useless.

If you can tell me which part of the code is not clear I'm going to fix this.

[zackurey]

@Sanne. Yes I'm perfectly happy with option 3. There is always the option of deprecating IndexEmbedded in the future if it turns out that in hindsight the community needs something more like option 1. And I agree that exclusions should be fine with option 3 as well.

[hferentschik]

(--)/ ...

[Sanne]

merged