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

GH-5090 Lucene 9 version of the Lucene SAIL #5091

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reckart
Copy link
Contributor

@reckart reckart commented Jul 26, 2024

GitHub issue resolved: #5090

Briefly describe the changes proposed in this PR:

  • Copied the Lucene SAIL implementation and upgraded it to Lucene 9

PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

- Copied the Lucene SAIL implementation and upgraded it to Lucene 9

Signed-off-by: Richard Eckart de Castilho <richard.eckart@gmail.com>
@reckart
Copy link
Contributor Author

reckart commented Jul 26, 2024

@hmottestad build fails due to the Lucene 9 dependencies apparently not being on the whitelist.

@hmottestad
Copy link
Contributor

hmottestad commented Jul 27, 2024

There is a license issue that the Eclipse IP-team needs to check for us. I'll try to look into that when I have some time.

The second seems to be a Javadocs issue. That might just be that we need to whitelist something, but I'm not sure. Could also be an issue with some transitive dependencies in Lucene 9.

@hmottestad
Copy link
Contributor

I'm still not very comfortable with having two Lucene sails. Mixing two versions of the same library has previously led to issues down the road. If someone was depending on the parent pom I would assume they would get both versions of the Lucene library.

@hmottestad
Copy link
Contributor

Some relevant discussion: #5037

@reckart
Copy link
Contributor Author

reckart commented Jul 27, 2024

If someone was depending on the parent pom I would assume they would get both versions of the Lucene library.

The maven resolver can only resolve a particular JAR to a single version, not to multiple.

If you inherit from the parent POM, you get the Lucene version defined there. Nobody should really depend on the parent POM. They should use the rdf4j-bom instead. But even if they depended on the parent POM, I don't see how they'd get Lucene version as a dependency and not even as a managed version.

The situation is somewhat similar to the rdf4j-rio-jsonld and rdf4j-rio-jsonld-legacy - the user has to make sure to only have one of those in their dependencies.

The new module locally overrides the Lucene version property - it won't leak. Presently, I didn't even add a managed dependency for the module to the BOM - something that actually should still be done.

@hmottestad
Copy link
Contributor

The json-ld side is a bit different. There we are depending on two completely different libraries. The only issue that would occur if you depend on both is that you wouldn't be sure which one you end up getting through the Java Service Provider Interface.

Here with Lucene we are depending on two versions of the same library.

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.

Lucene 9 version of the Lucene SAIL
2 participants