Remove the HPPC dependency from all modules and add the required classes to the hppc fork.

[bruno-roustant]

• Add the required classes to the hppc fork.
• Remove the hppc dependency from the facet, join, spatial modules.
• Remove the hppc version from versions.props and versions.lock.

It remains to move the hppc fork to oal.internal.

[uschindler]

Looks fine; i also like that you updated the classes to latest version which may have fixed some bugs.

There is one issue which already existed: The classes are public and also have no @lucene.internal in their javadocs. Maybe we should add this. The other suggestion was brought in by Dawid:

• Move the classes to org.apache.lucene.internal.hppc package which is not exported in module-info
• Add qualified exports to the module-info only for the Lucene modules which actually use the classes

Not sure if the move to internal package should be part of this PR, but adding the Javadocs comment to mark them as internal should be done here (for all classes, also those not modified).

[uschindler]

Ah I see you mentioned the missing move in the PR description, sorry!

So except the @lucene.internal javadocs (should be added always, also when in private package), +1

[dweiss]

I'll do it.

[bruno-roustant]

Ok, Dawid I let you move the fork.
I added the @lucene.internal annotations. I like that this hppc integration is cleaner and well defined now.

[bruno-roustant]

Do you want to move in this PR, or in another PR once this one is merged?

[dweiss]

I'll push to this PR, if you don't mind?

[ChrisHegarty]

Thank you for doing this. While there is still a little left to do in this PR, it looks like it's going in the right direction.

[dweiss]

So compilation works, but tests won't fly with this change. Recall we run tests in "classpath mode" so even though modules compile, for tests they're on classpath and then can't access internal packages from the core. Here's an example:

   >     java.lang.IllegalAccessError: class org.apache.lucene.search.join.DiversifyingNearestChildrenKnnCollector$NodeIdCachingHeap (in unnamed module @0x68472863) cannot access class org.apache.lucene.util.hppc.IntIntHashMap (in module org.apache.lucene.core) because module org.apache.lucene.core does not export org.apache.lucene.util.hppc to unnamed module @0x68472863
   >         at __randomizedtesting.SeedInfo.seed([9A5633A49FE14BF7:FC682151F0E8CD34]:0)
   >         at org.apache.lucene.search.join.DiversifyingNearestChildrenKnnCollector$NodeIdCachingHeap.<init>(DiversifyingNearestChildrenKnnCollector.java:132)
   >         at org.apache.lucene.search.join.DiversifyingNearestChildrenKnnCollector.<init>(DiversifyingNearestChildrenKnnCollector.java:47)
   >         at org.apache.lucene.search.join.DiversifyingNearestChildrenKnnCollectorManager.newCollector(DiversifyingNearestChildrenKnnCollectorManager.java:68)
   >         at org.apache.lucene.core@10.0.0-SNAPSHOT/org.apache.lucene.search.TimeLimitingKnnCollectorManager.newCollector(TimeLimitingKnnCollectorManager.java:41)

Fixing this requires going down the rabbit hole of compiling and running tests with module patching and/or moving tests to a separate module and reorganizing their internals so that there are no split packages. Invasive...

I wonder if the first baby step shouldn't be to move the hppc forked classes to .internal. (with proper annotation), but - sadly - export that package until we figure out how to compile and run tests in full modular mode.

[bruno-roustant]

I agree with the baby step, and this PR is already massive.

[dweiss]

I think the only thing missing is perhaps a changes entry and migration entry warning people that the dependency is gone and that the currently exposed internal package will be gone without a warning in the future (if we want to apply it to 9x as well).

[uschindler]

Looks ok as baby step.

About the test strategy: making tests modules is not a good idea, as this requires to open many more classes and it's also not the correct way for unit tests, which should run in same module.

Patching the module with test classes should work and is done by other projects, but I remember that you faced some issues.

Maybe another "hack": For running tests just pack a secondary jar file with main classes and tests including the module info? It's a duplicate but would make the tests and main classes a module with same name so all exports work.

Module patching should achieve the same, but I don't remember what your issues were.

At least for the MR-JAR compilation patching works, never tried at runtime.

[dweiss]

I can't remember what the problem was, to be honest. Will have to return to it.

[bruno-roustant]

Thank you Dawid!
I can work on the backport to 9x.

How long should we wait for this PR before merging it?
Given it is large, it has higher probabilities of conflicts, in main and 9x, so I would prefer to merge it quickly.

[dweiss]

I'd merge it right away, I don't see the point in waiting. If there are conflicts - they should be easy to resolve.

[uschindler]

Go ahead!

[bruno-roustant]

Backport to branch_9x complete!