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

LUCENE-10261: Remove preset analyzer panel from Luke Analysis UI. #475

Merged
merged 2 commits into from Nov 25, 2021

Conversation

mocobeta
Copy link
Contributor

@mocobeta mocobeta commented Nov 25, 2021

This removes "Preset analyzer" panel from the Luke Analysis UI so that we can throw away old reflection hacks that prevent adopting Java module system.

Analysis UI now only supports "Custom analyzer" panel - that definitely covers the whole functionality of dropped "Preset analyzer".

Screenshot from 2021-11-25 19-49-42

@dweiss
Copy link
Contributor

dweiss commented Nov 25, 2021

Hi @mocobeta ! Do you really want to remove it entirely? Maybe there is a way to make it work, somehow? This is not really a showstopper for the 9.0 release anyway - maybe we can figure out how to do it with modules later on?

@mocobeta
Copy link
Contributor Author

Personally it is sufficient for me to have Custom Analyzer in the Analysis UI, and I can't remember why I implement it.
I don't think we should waste developers' time on the problematic UI. It is easy to re-implement the same component once we come up with another way to enumerate provided Analysers (SPI?) in some module-friendly ways.

@dweiss
Copy link
Contributor

dweiss commented Nov 25, 2021

I don't know if you saw my comment here - https://issues.apache.org/jira/browse/LUCENE-10261 - this is quite mind-bending what happens in there. :)

@mocobeta mocobeta merged commit 40b3843 into apache:main Nov 25, 2021
@mocobeta mocobeta deleted the drop-preset-analyzer-panel branch November 25, 2021 11:30
@uschindler
Copy link
Contributor

I would just change the defaults. Show the custom view first and only allow to fallback to hard coded analyzers.

@@ -46,16 +43,13 @@
import org.apache.lucene.analysis.custom.CustomAnalyzer;
import org.apache.lucene.analysis.tokenattributes.CharTermAttribute;
import org.apache.lucene.luke.models.LukeException;
import org.apache.lucene.luke.util.reflection.ClassScanner;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this class still in use at other places in Luke?

Copy link
Contributor Author

@mocobeta mocobeta Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately. This is still used to list available directory implementations (when opening an index).
In 9.0, it still works - some Directorys that does not reside in lucene-core was gone from the list though.

private String[] supportedDirImpls() {
// supports FS-based built-in implementations
ClassScanner scanner = new ClassScanner("org.apache.lucene.store", getClass().getClassLoader());
Set<Class<? extends FSDirectory>> clazzSet = scanner.scanSubTypes(FSDirectory.class);

Copy link
Contributor Author

@mocobeta mocobeta Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uschindler @dweiss
There are only three FSDirectory implementations (in lucene-core) and it isn't likely to be added as Analyzers. Maybe we should hard code the class names (FQCNs) here, then remove ClassScanner from the module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There may only come a new mmapdir in future, but nothing else inside lucene. We have more directories in Sandbox or misc module, but those are special (wrappers on top of others).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleand it up in #480.

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