[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
dsmiley commented on a change in pull request #1109: More pervasive use of
PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361586269
##
File path: solr/core/src/java/org/apache/solr/core/PluginBag.java
##
@@ -140,11 +139,10 @@ public static void initInstance(Object inst, PluginInfo
info) {
log.debug("{} : '{}' created with startup=lazy ", meta.getCleanTag(),
info.name);
return new LazyPluginHolder(meta, info, core,
core.getResourceLoader(), false);
} else {
- if (info.pkgName != null) {
-PackagePluginHolder holder = new PackagePluginHolder<>(info, core,
meta);
-return holder;
+ if (core.getResourceLoader().getPackage(info.pkgName) != null) {
Review comment:
> The idea of SRL trying to resolve the correct package is wrong.
You seem pretty adamant about this; I suggest saying you don't like that
approach instead of it being "wrong". In this PR, SRL resolving the package is
a central theme and so if we don't agree on wether SRL should do this then this
PR has no future (ah well). Maybe we will return to this idea if there are
holes in other approaches.
I view the lack of correct version resolution in the PR (still only one
commit as I write this) as merely a necessary TODO, and I don't forsee it'd be
hard. If you would like me to explore adding this next, let me know.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361543380
##
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier getIndexSchemaSupplier() {
Review comment:
I figure you have this in support of hot-updating? Please state as such in
a comment.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361578602 ## File path: solr/core/src/java/org/apache/solr/core/SolrCore.java ## @@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo MDCLoggingContext.setCore(this); resourceLoader = config.getResourceLoader(); + resourceLoader.core = this; Review comment: @janhoy see ConfigSetService.createCoreResourceLoader This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361541789 ## File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java ## @@ -28,17 +30,17 @@ private final SolrConfig solrconfig; - private final IndexSchema indexSchema; + private volatile Supplier indexSchemaSupplier; Review comment: Why volatile and no longer final? I only see one write to this in the constructor. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361579553
##
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##
@@ -75,15 +78,32 @@ synchronized void
packagesUpdated(List pkgs) {
}
private synchronized void invokeListeners(PackageLoader.Package pkg) {
-for (Reference ref : listeners) {
- Listener listener = ref.get();
- if(listener == null) continue;
- if (listener.packageName() == null ||
listener.packageName().equals(pkg.name())) {
-listener.changed(pkg);
+Ctx ctx = new Ctx();
+
+try {
+ for (Reference ref : listeners) {
+Listener listener = ref.get();
+if(listener == null) continue;
+if (listener.packageName() == null ||
listener.packageName().equals(pkg.name())) {
+ listener.changed(pkg, ctx);
+}
+ }
+} finally {
+ if(ctx.postProcessors != null){
+for (Runnable value : ctx.postProcessors.values()) {
+ value.run();
+}
}
}
}
+ public void forEachListener(Consumer listenerConsumer){
+listeners.forEach(ref -> {
Review comment:
Consider this instead:
```listeners.stream().map(Reference::get).filter(Objects::nonNull).forEach(listenerConsumer);```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361542052
##
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map attrs,
NamedList initArgs, Li
/** class names can be prefixed with package name e.g:
my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair parseClassName(String name) {
-String pkgName = null;
-String className = name;
-if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
-pkgName = name.substring(0, colonIdx);
-className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
I suggest "ParsedClass" consisting of "pkg", "clazz", and "original" (not
"actual"). Though I'm not sure I like the very existence of the class; I'd
rather it be private if it needs to exist.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361543861
##
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier getIndexSchemaSupplier() {
Review comment:
Based on what I'm seeing in this PR, it appears this Supplier will in fact
load the schema each time get() is called? If so that sounds easy to
accidentally over-load the schema. Might a `Future` work better?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361542449 ## File path: solr/core/src/java/org/apache/solr/core/PluginLoader.java ## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.core; + +/**A generic interface to load classes Review comment: Can you please either (A) put the first javadoc sentence on the following line and not adjacent to the asterisk *or* (B) have the entire javadoc syntax be the one line since it's very short. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14102) FacetModule use number of responses from shards as number of shards
[ https://issues.apache.org/jira/browse/SOLR-14102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003833#comment-17003833 ] Tomohiro Manabe commented on SOLR-14102: Thanks for reviewing. I think we need a mock SearchComponent for asserting this problem. Is this plan fine with you? > FacetModule use number of responses from shards as number of shards > --- > > Key: SOLR-14102 > URL: https://issues.apache.org/jira/browse/SOLR-14102 > Project: Solr > Issue Type: Bug > Security Level: Public(Default Security Level. Issues are Public) >Reporter: Tomohiro Manabe >Priority: Major > Attachments: SOLR-14102.patch > > > Hi, I am developing a SearchComponent which sends some requests to only a > part of shards. > Basically it works well, but using it with facetting triggers an error. > I found that FacetModule use the number of responses from shards > (sreq.responses.size()) as the number of shards. > Of course this assumption is incorrect in case that only a part of shards > respond. > Instead of the number of responses, using rb.shards.length may be better. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569156544 > I like the 3 categories of plugins very much, and also especially glad and surprised to hear you state "Hot reloading is not important." The reason why I went with hot reloading for certain components was that it was easy to implement and tests were less likely to fail. My experience with `runtimeLib` taught me that core reloads are not very reliable during tests. They fail sporadically. Moreover, we should aim to reload the smallest possible entity for sake of efficiency and stability. For instance, reloading an instance of a `SearchComponent` may happen in sub milliseconds, but a core reload can take an unpredictable amount of time depending on various other factors The idea of SRL resolving the class name to appropriate package works, but it's hard to get it right. Hot reloading is not the problem, consistency is. Consider the following - packages can be updated any time - cores can be reloaded any time - nodes can be restarted at any time The challenge is to ensure that every single plugin in every single node in the cluster has the same version of the package being used. If we cannot have a mechanism to somehow reload the plugin, we cannot ensure consistency. Every time the package is updated, we must ensure that the plugin is refreshed. SRL is generic. It doesn't know how to reload some plugin optimally. Today, all the implementations treat the plugin itself as the atomic unit (even then there are 3 types : Search components, Streaming expressions and other components). #1124 treats the `IndexSchema` as the atomic unit. We may have another set of plugins that use the `SolrCore` as the atomic unit. There are so many ways we can do this. But, we CANNOT say that we will somehow load the classes today and we will worry about reloading it later. It works fine for a single node Solr , it cannot work correctly for a cluster of nodes. So, when I say something is "plain wrong", it just means that the design choices are making it impossible for us to give appropriate consistency guarantees for the entire system. Then we have another often overlooked factor of "diagnosability". How do we give the user visibility into what version of the plugin is used in a given node/core? There should be an appropriate end point where people can probe the system to get that information. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361541004
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
+if (fragmentEndFromPreceding == DONE) {
+ fragmentEndFromPreceding = baseIter.last();
+}
+final int centerLength = fragmentEndFromPreceding - fragmentStart;
+final int extraPrecedingLengthGoal = (int)((lengthGoal - centerLength) *
fragmentAlignment);
Review comment:
BTW see Solr's "LuceneRegexFragmenter" which has a "slop" notion and may
have ideas worth looking at.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361539250
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
+if (fragmentEndFromPreceding == DONE) {
+ fragmentEndFromPreceding = baseIter.last();
+}
+final int centerLength = fragmentEndFromPreceding - fragmentStart;
+final int extraPrecedingLengthGoal = (int)((lengthGoal - centerLength) *
fragmentAlignment);
Review comment:
I'm glad you're cool with entertaining reworking the whole thing :-) BTW my
proposal is rather incomplete or imperfect or both. We probably won't get a
perfect lengthGoal hit (hence "goal" in the name). And we almost certainly
won't get a perfect fragmentAlignment ratio with before/after -- it should
perhaps also have "goal" in the name. So whatever the solution is will have
some inherent fuzziness to it. This is probably all obvious to you but I just
want to state I'm aware you can poke lots of holes in my proposal. I just
think we can do a bit better than your original proposal by using some more
information.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361538041 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/FieldHighlighter.java ## @@ -159,8 +160,9 @@ public Object highlightFieldForDoc(LeafReader reader, int docId, String content) break; } // advance breakIterator -passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), 0)); -passage.setEndOffset(Math.min(this.breakIterator.following(start), contentLength)); +passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), lastPassageEnd)); +lastPassageEnd = Math.min(this.breakIterator.following(start), contentLength); Review comment: RE "end": I should have made it clear that I make the small request in awareness it will not have any value when this particular LengthGoalBreakIterator is used, at least as is written now. But in principle it makes sense and who knows what BI a user might supply. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361535945
##
File path:
lucene/highlighter/src/test/org/apache/lucene/search/uhighlight/LengthGoalBreakIteratorTest.java
##
@@ -41,6 +43,31 @@
// 01234567890123456789
static final String CONTENT = "Aa bb. Cc dd. Ee ff";
+ public void testFragmentAlignmentConstructor() throws IOException {
+BreakIterator baseBI = new CustomSeparatorBreakIterator('.');
+// test fragmentAlignment validation
+float[] valid_aligns = {0.f, 0.f, 0.5f, 0.99f, 1.f};
+for (float alignment : valid_aligns) {
+ LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+}
+float[] invalid_aligns = {-0.01f, -1.f, 1.5f, Float.NaN,
Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY};
+for (float alignment : invalid_aligns) {
+ try {
+LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+Assert.fail("Expected IllegalArgumentException for "+alignment);
+ } catch (IllegalArgumentException e) {
+if (!e.getMessage().contains("fragmentAlignment")) {
+ throw e;
+}
+ }
+}
+// test backwards compatibility constructors
+String backwardCompString =
LengthGoalBreakIterator.createClosestToLength(baseBI, 50).toString();
Review comment:
It's fine. I can't say I would have bothered but you did it and the test
has value.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361533868
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
+if (fragmentEndFromPreceding == DONE) {
+ fragmentEndFromPreceding = baseIter.last();
+}
+final int centerLength = fragmentEndFromPreceding - fragmentStart;
+final int extraPrecedingLengthGoal = (int)((lengthGoal - centerLength) *
fragmentAlignment);
Review comment:
On the contrary!
This suggestion is a little different than what I've thought of, so I'll
have to rework the whole thing, but it has a great quality: This would make the
length of the match irrelevant for the GLBI logic. I'm not going to go through
all the details, but the code can be a little more optimal and less... hacky.
I should have read this before writing [my previous
comment](https://github.com/apache/lucene-solr/pull/1123#discussion_r361529545),
now it is deprecated.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361530659 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/FieldHighlighter.java ## @@ -159,8 +160,9 @@ public Object highlightFieldForDoc(LeafReader reader, int docId, String content) break; } // advance breakIterator -passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), 0)); Review comment: Even if this was not necessary before, the current version with `lastPassageEnd` does need `max()`. Without it the passages could overlap. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361529545 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/FieldHighlighter.java ## @@ -159,8 +160,9 @@ public Object highlightFieldForDoc(LeafReader reader, int docId, String content) break; } // advance breakIterator -passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), 0)); -passage.setEndOffset(Math.min(this.breakIterator.following(start), contentLength)); +passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), lastPassageEnd)); +lastPassageEnd = Math.min(this.breakIterator.following(start), contentLength); Review comment: Changing the `following(start)` to `following(end - 1)` would have been a great idea before this changeset. However these two calls on the `LengthGoalBreakIterator` are very specifically crafted. The `preceding()` prepares state data that `following()` will use. Currently the workflow is: 1. The call to `preceding(start + 1)` will identify the `startIdx` and `endIdx` around the match by the underlying BI. This `startIdx` can be earlier than the start of the match. Similarly this `endIdx` can be later than the end of the match. (this is the usual case for SENTENCE BI) Both are needed at this point to know how long the match is. Then `fragsize-(endIdx-startIdx)` is the remaining size to give as context around the first fragment. Then `context_size*fragalign` is the length `preceding()` will try to prepend to the fragment. Finally it will return the final start position. 2. The call to `following(start)` ignores the argument because the end of the match's fragment (`endIdx`) was already found in `preceding()`. `following()` will try to append the missing length also prepared by `preceding()`. Then return the final end position. This is what I was referring to in my comment when I submitted the first version of the patch. Ideally there was a single method on the `LengthGoalBreakIterator` that would receive the start **and** end positions of the match to work with. That way the processing would be more optimal. The underlying BI would not need to iterate over the content from start to end when searching for the `endIdx`. It could start from `end - 1` of the match. I have tried to change up this part of the code more, but a whole bunch of tests broke. I think there is some other BreakIterator subclass that gets passed around here. So this part can't be rewritten to cater to this specific behavior without greater changes. Finally, `max(preceding(start + 1) ,lastPassageEnd)` is needed because the `preceding()` call can go backwards and create overlaps with the previous passage. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-13101) Shared storage support in SolrCloud
[ https://issues.apache.org/jira/browse/SOLR-13101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003748#comment-17003748 ] ASF subversion and git services commented on SOLR-13101: Commit ef01979c6484012e1369cb2aedf927fd58152709 in lucene-solr's branch refs/heads/jira/SOLR-13101 from Megan Carey [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=ef01979 ] SOLR-13101: ant precommit fixes (#1117) * Fix the gson version reference * Fixed all precommit failures Co-authored-by: Andy Vuong > Shared storage support in SolrCloud > --- > > Key: SOLR-13101 > URL: https://issues.apache.org/jira/browse/SOLR-13101 > Project: Solr > Issue Type: New Feature > Components: SolrCloud >Reporter: Yonik Seeley >Priority: Major > Time Spent: 8h 40m > Remaining Estimate: 0h > > Solr should have first-class support for shared storage (blob/object stores > like S3, google cloud storage, etc. and shared filesystems like HDFS, NFS, > etc). > The key component will likely be a new replica type for shared storage. It > would have many of the benefits of the current "pull" replicas (not indexing > on all replicas, all shards identical with no shards getting out-of-sync, > etc), but would have additional benefits: > - Any shard could become leader (the blob store always has the index) > - Better elasticity scaling down >- durability not linked to number of replcias.. a single replica could be > common for write workloads >- could drop to 0 replicas for a shard when not needed (blob store always > has index) > - Allow for higher performance write workloads by skipping the transaction > log >- don't pay for what you don't need >- a commit will be necessary to flush to stable storage (blob store) > - A lot of the complexity and failure modes go away > An additional component a Directory implementation that will work well with > blob stores. We probably want one that treats local disk as a cache since > the latency to remote storage is so large. I think there are still some > "locking" issues to be solved here (ensuring that more than one writer to the > same index won't corrupt it). This should probably be pulled out into a > different JIRA issue. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] yonik merged pull request #1117: SOLR-13101: ant precommit fixes
yonik merged pull request #1117: SOLR-13101: ant precommit fixes URL: https://github.com/apache/lucene-solr/pull/1117 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] yonik merged pull request #1115: SOLR-13101: Fix the gson version reference
yonik merged pull request #1115: SOLR-13101: Fix the gson version reference URL: https://github.com/apache/lucene-solr/pull/1115 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] andyvuong commented on a change in pull request #1117: SOLR-13101: ant precommit fixes
andyvuong commented on a change in pull request #1117: SOLR-13101: ant
precommit fixes
URL: https://github.com/apache/lucene-solr/pull/1117#discussion_r361501107
##
File path:
solr/core/src/java/org/apache/solr/store/blob/client/LocalStorageClient.java
##
@@ -97,7 +117,7 @@ public void pushCoreMetadata(String sharedStoreName, String
blobCoreMetadataName
// Writing to the file assumed atomic, the file cannot be observed
midway. Might not hold here but should be the case
// with a real S3 implementation.
- try (PrintWriter out = new PrintWriter(blobMetadataFile)){
+ try (PrintWriter out = new PrintWriter(blobMetadataFile,
StandardCharsets.UTF_8.name())){
Review comment:
This only affects core.metadata and not all blob files btw - local storage
client is only used for testing purposes so this is fine here too.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14141) eliminate JKS keystore from solr SSL docs
[
https://issues.apache.org/jira/browse/SOLR-14141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003705#comment-17003705
]
Robert Muir commented on SOLR-14141:
I added explicit {{-storetype PKCS12}}, for java 8. Java 8 has the compat
change ( https://openjdk.java.net/jeps/166 ) but the defaults change did not
happen until java 9: https://openjdk.java.net/jeps/229
> eliminate JKS keystore from solr SSL docs
> -
>
> Key: SOLR-14141
> URL: https://issues.apache.org/jira/browse/SOLR-14141
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
>Reporter: Robert Muir
>Priority: Major
> Attachments: SOLR-14141.patch, SOLR-14141.patch
>
>
> On the "Enabling SSL" page:
> https://lucene.apache.org/solr/guide/8_3/enabling-ssl.html#enabling-ssl
> The first step is currently to create a JKS keystore. The next step
> immediately converts the JKS keystore into PKCS12, so that openssl can then
> be used to extract key material in PEM format for use with curl.
> Now that PKCS12 is java's default keystore format, why not omit step 1
> entirely? What am I missing? PKCS12 is a more commonly
> understood/standardized format.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Updated] (SOLR-14141) eliminate JKS keystore from solr SSL docs
[ https://issues.apache.org/jira/browse/SOLR-14141?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Robert Muir updated SOLR-14141: --- Attachment: SOLR-14141.patch > eliminate JKS keystore from solr SSL docs > - > > Key: SOLR-14141 > URL: https://issues.apache.org/jira/browse/SOLR-14141 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: Robert Muir >Priority: Major > Attachments: SOLR-14141.patch, SOLR-14141.patch > > > On the "Enabling SSL" page: > https://lucene.apache.org/solr/guide/8_3/enabling-ssl.html#enabling-ssl > The first step is currently to create a JKS keystore. The next step > immediately converts the JKS keystore into PKCS12, so that openssl can then > be used to extract key material in PEM format for use with curl. > Now that PKCS12 is java's default keystore format, why not omit step 1 > entirely? What am I missing? PKCS12 is a more commonly > understood/standardized format. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361492729
##
File path:
lucene/highlighter/src/test/org/apache/lucene/search/uhighlight/LengthGoalBreakIteratorTest.java
##
@@ -41,6 +43,31 @@
// 01234567890123456789
static final String CONTENT = "Aa bb. Cc dd. Ee ff";
+ public void testFragmentAlignmentConstructor() throws IOException {
+BreakIterator baseBI = new CustomSeparatorBreakIterator('.');
+// test fragmentAlignment validation
+float[] valid_aligns = {0.f, 0.f, 0.5f, 0.99f, 1.f};
+for (float alignment : valid_aligns) {
+ LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+}
+float[] invalid_aligns = {-0.01f, -1.f, 1.5f, Float.NaN,
Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY};
+for (float alignment : invalid_aligns) {
+ try {
+LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+Assert.fail("Expected IllegalArgumentException for "+alignment);
+ } catch (IllegalArgumentException e) {
+if (!e.getMessage().contains("fragmentAlignment")) {
+ throw e;
+}
+ }
+}
+// test backwards compatibility constructors
+String backwardCompString =
LengthGoalBreakIterator.createClosestToLength(baseBI, 50).toString();
Review comment:
I wasn't sure to write those or not. Are they an overkill?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361491499
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -33,23 +33,45 @@
private final BreakIterator baseIter;
private final int lengthGoal;
+ private final float fragmentAlignment; // how much text to align before
match-fragment, valid in range [0, 1]
private final boolean isMinimumLength; // if false then is "closest to"
length
+ private int fragmentEndFromPreceding; // store the match-break end for reuse
in following()
+ private int fragmentEndFollowingLengthGoalFromPreceding; // store the
remaining length to collect in following()
/** Breaks will be at least {@code minLength} apart (to the extent
possible). */
+ public static LengthGoalBreakIterator createMinLength(BreakIterator
baseIter, int minLength,
+float
fragmentAlignment) {
+return new LengthGoalBreakIterator(baseIter, minLength,
fragmentAlignment,true);
+ }
+
+ /** For backwards compatibility you can initialise the break iterator
without fragmentAlignment. */
public static LengthGoalBreakIterator createMinLength(BreakIterator
baseIter, int minLength) {
-return new LengthGoalBreakIterator(baseIter, minLength, true);
+return createMinLength(baseIter, minLength, 0.f);
}
/** Breaks will be on average {@code targetLength} apart; the closest break
to this target (before or after)
* is chosen. */
+ public static LengthGoalBreakIterator createClosestToLength(BreakIterator
baseIter, int targetLength,
+ float
fragmentAlignment) {
+return new LengthGoalBreakIterator(baseIter, targetLength,
fragmentAlignment, false);
+ }
+
+ /** For backwards compatibility you can initialise the break iterator
without fragmentAlignment. */
public static LengthGoalBreakIterator createClosestToLength(BreakIterator
baseIter, int targetLength) {
-return new LengthGoalBreakIterator(baseIter, targetLength, false);
+return createClosestToLength(baseIter, targetLength, 0.f);
}
- private LengthGoalBreakIterator(BreakIterator baseIter, int lengthGoal,
boolean isMinimumLength) {
+ private LengthGoalBreakIterator(BreakIterator baseIter, int lengthGoal,
float fragmentAlignment,
+ boolean isMinimumLength) {
this.baseIter = baseIter;
this.lengthGoal = lengthGoal;
+if (fragmentAlignment < 0.f || fragmentAlignment > 1.f ||
!Float.isFinite(fragmentAlignment)) {
+ throw new IllegalArgumentException("fragmentAlignment must be >= zero
and <= one");
+}
+this.fragmentAlignment = Math.max(Math.min(fragmentAlignment, 1.f), 0.f);
Review comment:
I was too preoccupied with the testing, so I forgot about that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
dsmiley commented on issue #1109: More pervasive use of PackageLoader / PluginInfo URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569088078 I like the 3 categories of plugins very much, and also especially glad and surprised to hear you state "Hot reloading is not important.". Eventually we'll want to document this in some way by-plugin abstraction so users understand in the ref guide. And if you try to refer to a plugin when we know we can't support it, then we should tell the user more explicitly so. I still need to look at your schema PR #1124 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
dsmiley commented on a change in pull request #1109: More pervasive use of
PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361490021
##
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String
resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className,
IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info,
IndexSchemaFactory.class);
Review comment:
> am I supposed to make comments on what it does today?
Yes (agreed); and not the future.
> I never said SRL loading classes is "plain wrong
I'm glad to hear that. You made this statement immediately after what I
said, which was "It does! Woohoo! It's because all loading occurs via a
SolrResourceLoader in Solr and so that's where the focal point of the approach
in this PR is." So I thought you meant my most recent statement "was plain
wrong" vs some statement in some other comment thread (notice this thread
doesn't discuss versions). Can you see how I came to this interpretation?
I am working this week, albeit lightly BTW.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361476978 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/FieldHighlighter.java ## @@ -159,8 +160,9 @@ public Object highlightFieldForDoc(LeafReader reader, int docId, String content) break; } // advance breakIterator -passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), 0)); -passage.setEndOffset(Math.min(this.breakIterator.following(start), contentLength)); +passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), lastPassageEnd)); +lastPassageEnd = Math.min(this.breakIterator.following(start), contentLength); Review comment: What I'm about to propose is perhaps technically out of scope but can you please call `following` with the variable `end` minus 1? Do you think we might need to guard the passage end offset by ensuring it is > the start offset? start might theoretically be before lastPassageEnd I think; if it wasn't possible then you wouldn't have guarded that with `max(...,lastPassageEnd)` This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361471705
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -44,17 +44,30 @@ public static LengthGoalBreakIterator
createMinLength(BreakIterator baseIter, in
return new LengthGoalBreakIterator(baseIter, minLength,
fragmentAlignment,true);
}
+ /** For backwards compatibility you can initialise the break iterator
without fragmentAlignment. */
+ public static LengthGoalBreakIterator createMinLength(BreakIterator
baseIter, int minLength) {
+return createMinLength(baseIter, minLength, 0.f);
+ }
+
/** Breaks will be on average {@code targetLength} apart; the closest break
to this target (before or after)
* is chosen. */
public static LengthGoalBreakIterator createClosestToLength(BreakIterator
baseIter, int targetLength,
Review comment:
Please document fragmentAlignment
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361471554 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java ## @@ -44,17 +44,30 @@ public static LengthGoalBreakIterator createMinLength(BreakIterator baseIter, in return new LengthGoalBreakIterator(baseIter, minLength, fragmentAlignment,true); } + /** For backwards compatibility you can initialise the break iterator without fragmentAlignment. */ Review comment: With \@Deprecated This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361472276
##
File path:
lucene/highlighter/src/test/org/apache/lucene/search/uhighlight/LengthGoalBreakIteratorTest.java
##
@@ -41,6 +43,31 @@
// 01234567890123456789
static final String CONTENT = "Aa bb. Cc dd. Ee ff";
+ public void testFragmentAlignmentConstructor() throws IOException {
+BreakIterator baseBI = new CustomSeparatorBreakIterator('.');
+// test fragmentAlignment validation
+float[] valid_aligns = {0.f, 0.f, 0.5f, 0.99f, 1.f};
+for (float alignment : valid_aligns) {
+ LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+}
+float[] invalid_aligns = {-0.01f, -1.f, 1.5f, Float.NaN,
Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY};
+for (float alignment : invalid_aligns) {
+ try {
+LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+Assert.fail("Expected IllegalArgumentException for "+alignment);
+ } catch (IllegalArgumentException e) {
Review comment:
See `LuceneTestCase.exceptThrows`
BTW this is very thorough of you to test this
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361486777
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
+if (fragmentEndFromPreceding == DONE) {
+ fragmentEndFromPreceding = baseIter.last();
+}
+final int centerLength = fragmentEndFromPreceding - fragmentStart;
+final int extraPrecedingLengthGoal = (int)((lengthGoal - centerLength) *
fragmentAlignment);
Review comment:
I'm noticing that the logic here for fragment alignment doesn't seem to care
whatsoever about where "offset" is within it's segment. But isn't that super
relevant? As I consider a BI on SENTENCE based segments, I think it is... but
I can see how you overlooked this when focusing on WORD scenarios. For example
assuming 0.5 fragmentAlignment, and if the "offset" (the match) happens to
occur at the right end of the segment, and lets say the length goal is only 10
chars larger than centerLength, then shouldn't we expand to the right and not
the left?
You're going to hate me for this but let me make a proposal :-) What if we
multiply fragmentAlignment by lengthGoal and interpret this as a minimum number
of characters wanted to the left of the start of the match (`offset`). The
difference of that with lengthGoal indicates minimum chars wanted to the right.
We use the delegate BI to find the passage start, and then we can consult
fragmentAlignment with where `offset` is relative to the start to decide how
much text to the right of `offset` we want.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361483326
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
+if (fragmentEndFromPreceding == DONE) {
+ fragmentEndFromPreceding = baseIter.last();
+}
+final int centerLength = fragmentEndFromPreceding - fragmentStart;
Review comment:
All this is inherently confusing so lets add a bit more comments or
reconsider some variable names. It would help me understand "centerLength"
with a comment above saying something like "centerLength is the length of the
center-most segment.". Or maybe "span" and not segment; too bad there isn't a
clear word for these. Could be a sentence, word, or something else; its
whatever the BI is breaking.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361480777 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/FieldHighlighter.java ## @@ -159,8 +160,9 @@ public Object highlightFieldForDoc(LeafReader reader, int docId, String content) break; } // advance breakIterator -passage.setStartOffset(Math.max(this.breakIterator.preceding(start + 1), 0)); Review comment: The "max" with 0 had defended against the DONE scenario. I'm honestly not sure if DONE is possible. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361472624 ## File path: lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java ## @@ -44,17 +44,30 @@ public static LengthGoalBreakIterator createMinLength(BreakIterator baseIter, in return new LengthGoalBreakIterator(baseIter, minLength, fragmentAlignment,true); Review comment: super minor: please add a space after the comma This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361472335
##
File path:
lucene/highlighter/src/test/org/apache/lucene/search/uhighlight/LengthGoalBreakIteratorTest.java
##
@@ -41,6 +43,31 @@
// 01234567890123456789
static final String CONTENT = "Aa bb. Cc dd. Ee ff";
+ public void testFragmentAlignmentConstructor() throws IOException {
+BreakIterator baseBI = new CustomSeparatorBreakIterator('.');
+// test fragmentAlignment validation
+float[] valid_aligns = {0.f, 0.f, 0.5f, 0.99f, 1.f};
+for (float alignment : valid_aligns) {
+ LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+}
+float[] invalid_aligns = {-0.01f, -1.f, 1.5f, Float.NaN,
Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY};
+for (float alignment : invalid_aligns) {
+ try {
+LengthGoalBreakIterator.createClosestToLength(baseBI, 50, alignment);
+Assert.fail("Expected IllegalArgumentException for "+alignment);
+ } catch (IllegalArgumentException e) {
+if (!e.getMessage().contains("fragmentAlignment")) {
+ throw e;
+}
+ }
+}
+// test backwards compatibility constructors
+String backwardCompString =
LengthGoalBreakIterator.createClosestToLength(baseBI, 50).toString();
Review comment:
Impressively thorough
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361482118
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -33,23 +33,45 @@
private final BreakIterator baseIter;
private final int lengthGoal;
+ private final float fragmentAlignment; // how much text to align before
match-fragment, valid in range [0, 1]
private final boolean isMinimumLength; // if false then is "closest to"
length
+ private int fragmentEndFromPreceding; // store the match-break end for reuse
in following()
+ private int fragmentEndFollowingLengthGoalFromPreceding; // store the
remaining length to collect in following()
/** Breaks will be at least {@code minLength} apart (to the extent
possible). */
+ public static LengthGoalBreakIterator createMinLength(BreakIterator
baseIter, int minLength,
+float
fragmentAlignment) {
+return new LengthGoalBreakIterator(baseIter, minLength,
fragmentAlignment,true);
+ }
+
+ /** For backwards compatibility you can initialise the break iterator
without fragmentAlignment. */
public static LengthGoalBreakIterator createMinLength(BreakIterator
baseIter, int minLength) {
-return new LengthGoalBreakIterator(baseIter, minLength, true);
+return createMinLength(baseIter, minLength, 0.f);
}
/** Breaks will be on average {@code targetLength} apart; the closest break
to this target (before or after)
* is chosen. */
+ public static LengthGoalBreakIterator createClosestToLength(BreakIterator
baseIter, int targetLength,
+ float
fragmentAlignment) {
+return new LengthGoalBreakIterator(baseIter, targetLength,
fragmentAlignment, false);
+ }
+
+ /** For backwards compatibility you can initialise the break iterator
without fragmentAlignment. */
public static LengthGoalBreakIterator createClosestToLength(BreakIterator
baseIter, int targetLength) {
-return new LengthGoalBreakIterator(baseIter, targetLength, false);
+return createClosestToLength(baseIter, targetLength, 0.f);
}
- private LengthGoalBreakIterator(BreakIterator baseIter, int lengthGoal,
boolean isMinimumLength) {
+ private LengthGoalBreakIterator(BreakIterator baseIter, int lengthGoal,
float fragmentAlignment,
+ boolean isMinimumLength) {
this.baseIter = baseIter;
this.lengthGoal = lengthGoal;
+if (fragmentAlignment < 0.f || fragmentAlignment > 1.f ||
!Float.isFinite(fragmentAlignment)) {
+ throw new IllegalArgumentException("fragmentAlignment must be >= zero
and <= one");
+}
+this.fragmentAlignment = Math.max(Math.min(fragmentAlignment, 1.f), 0.f);
Review comment:
Why the max & min at this point?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361480642
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
Review comment:
AH I see! I somehow misinterpreted your question. You're mostly correct. The
`baseIter.following(fragmentStart)` should be `baseIter.following(offset - 1)`.
The minus one is needed for the very unlikely edge case where the match is a
single character and the user requests the absolute minimal length for
highlight. I added a little test that checks for that, you can see for yourself.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14151) Make schema components load from packages
[
https://issues.apache.org/jira/browse/SOLR-14151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003668#comment-17003668
]
Jan Høydahl commented on SOLR-14151:
i was thinking about refGuide updates, might be natural to add a note that when
loading from packages “class” must be used if spi is not supported.
> Make schema components load from packages
> -
>
> Key: SOLR-14151
> URL: https://issues.apache.org/jira/browse/SOLR-14151
> Project: Solr
> Issue Type: Sub-task
> Security Level: Public(Default Security Level. Issues are Public)
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Example:
> {code:xml}
>
>
>
>generateNumberParts="0" catenateWords="0"
> catenateNumbers="0" catenateAll="0"/>
>
>
>
>
> {code}
> * When a package is updated, the entire {{IndexSchema}} object is refreshed,
> but the SolrCore object is not reloaded
> * Any component can be prefixed with the package name
> * The semantics of loading plugins remain the same as that of the components
> in {{solrconfig.xml}}
> * Plugins can be registered using schema API
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
dsmiley commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361471274
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
Review comment:
I understand what `offset` param is and what `fragmentStart` is, and that
the boundaries of each are in different universes that may or may not align.
But I don't see why that matters here. I claim the call to
`baseIter.following(fragmentStart)` is guaranteed to be equal to
`baseIter.following(offset)` _because_ fragmentStart is
`baseIter.preceding(offset)`. The argument to the breakIterator can be any
arbitrary character offset; it needn't be an existing break of any kind. FWIW
I made the simple correction and tests pass. If you insist there is a problem
then can you present a failing test?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (LUCENE-4702) Terms dictionary compression
[
https://issues.apache.org/jira/browse/LUCENE-4702?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003640#comment-17003640
]
Adrien Grand commented on LUCENE-4702:
--
I finally explored a different path: JDK13 added more auto-vectorization
optimizations on byte[] arrays, so I wanted to look into whether we could
leverage it for compression. I ended up with a few lines of code that can
encode/decode byte[] arrays with a compression ratio of ~75%, if most (there is
support for exceptions) bytes are either in the [0x1F,0x3F) or [0x5F,0x7F)
ranges, which notably include all digits, lowercase characters, '.', '-' and
'_'. So it should be applicable most of the time to terms dictionaries of
analyzed content. It already helps on our nightly benchmarks, even though very
little normalization is performed (e.g. no ascii folding). It is usually faster
than LZ4 for short sequences of text (several times faster on JDK13+, and a bit
faster on previous JDKs), like our blocks of suffixes.
LZ4's ability to remove duplicate strings is still helpful, but since it hurts
multi-term queries I only enabled it when it yields compression ratios that are
less than 75%.
I got the following results on a force-merged wikibigall (note that results are
not comparable at all with previous results on this issue, since this is a
different dataset and that there have been many other changes in Lucene that
affect theses benchmarks, especially the fact that benchmarks now only count
1,000 hits):
{noformat}
TaskQPS baseline StdDev QPS patch StdDev
Pct diff
Respell 164.33 (6.7%) 140.08 (4.3%)
-14.8% ( -24% - -4%)
Fuzzy2 108.19 (7.7%) 101.51 (6.6%)
-6.2% ( -19% -8%)
Wildcard 94.23 (2.8%) 88.42 (2.6%)
-6.2% ( -11% -0%)
Prefix3 247.07 (5.1%) 244.95 (4.0%)
-0.9% ( -9% -8%)
TermBGroup1M 24.38 (6.4%) 24.17 (6.3%)
-0.8% ( -12% - 12%)
TermGroup1M 23.12 (6.6%) 23.02 (6.0%)
-0.4% ( -12% - 13%)
AndHighHigh 35.88 (4.8%) 35.78 (5.0%)
-0.3% ( -9% -9%)
TermGroup10K 45.63 (5.7%) 45.53 (5.4%)
-0.2% ( -10% - 11%)
SpanNear 10.89 (1.4%) 10.87 (1.5%)
-0.2% ( -3% -2%)
SloppyPhrase 19.57 (4.1%) 19.54 (4.1%)
-0.1% ( -8% -8%)
Phrase 69.13 (3.5%) 69.05 (3.9%)
-0.1% ( -7% -7%)
AndHighMed 50.75 (4.6%) 50.70 (4.6%)
-0.1% ( -8% -9%)
IntervalsOrdered 23.97 (0.8%) 23.96 (0.6%)
-0.0% ( -1% -1%)
Term 1432.69 (3.8%) 1432.25 (3.7%)
-0.0% ( -7% -7%)
AndHighOrMedMed 37.71 (1.7%) 37.72 (1.7%)
0.0% ( -3% -3%)
TermBGroup1M1P 25.61 (3.4%) 25.62 (3.4%)
0.1% ( -6% -7%)
TermDTSort 41.04 (4.9%) 41.06 (4.6%)
0.1% ( -9% - 10%)
OrHighMed 35.05 (3.2%) 35.08 (3.4%)
0.1% ( -6% -6%)
AndMedOrHighHigh 34.22 (3.5%) 34.26 (3.7%)
0.1% ( -6% -7%)
TermDayOfYearSort 93.34 (7.6%) 93.60 (7.2%)
0.3% ( -13% - 16%)
TermGroup100 15.21 (3.1%) 15.27 (3.0%)
0.4% ( -5% -6%)
TermMonthSort 49.27 (2.7%) 49.53 (2.3%)
0.5% ( -4% -5%)
TermTitleSort 127.41 (2.8%) 128.12 (2.2%)
0.6% ( -4% -5%)
OrHighHigh 10.14 (3.3%) 10.20 (3.5%)
0.6% ( -5% -7%)
Fuzzy1 159.76 (8.2%) 161.68 (6.6%)
1.2% ( -12% - 17%)
IntNRQ 266.89 (8.8%) 280.44 (11.6%)
5.1% ( -14% - 27%)
{noformat}
The hit on {{Respell}} is significant, but on other multi-term queries it looks
reasonable to me. It gave a ~9.3% reduction of the {{tim}} file, from 937MB to
850MB. Here are the detailed stats for the "body" field:
{noformat}
index FST:
72 bytes
terms:
46916528 terms
595069147 bytes (12.7 bytes/term)
blocks:
1507239 blocks
1158537 terms-only blocks
471 sub-block-only blocks
348231 mixed blocks
318391 floor blocks
491775 non-floor blocks
1015464 floor sub-blocks
359890173 term suffix bytes before compression (196.4 suffix-bytes/block)
296029380 compressed term suffix bytes (0.82 compression ratio -
compression count by algorithm: uncompressed:225133, lowercase_ascii:1217151,
LZ4:64955)
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361455949
##
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map attrs,
NamedList initArgs, Li
/** class names can be prefixed with package name e.g:
my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair parseClassName(String name) {
-String pkgName = null;
-String className = name;
-if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
-pkgName = name.substring(0, colonIdx);
-className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
I didn't like the name either. Suggestions welcome
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361456012
##
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f ->
PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
defensive programming
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361456012
##
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f ->
PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
defensive , programming
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361455888 ## File path: solr/core/src/java/org/apache/solr/core/SolrCore.java ## @@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo MDCLoggingContext.setCore(this); resourceLoader = config.getResourceLoader(); + resourceLoader.core = this; Review comment: NO, SRL is per core This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] jpountz opened a new pull request #1126: LUCENE-5201: Terms dictionary compression.
jpountz opened a new pull request #1126: LUCENE-5201: Terms dictionary compression. URL: https://github.com/apache/lucene-solr/pull/1126 Compress blocks of suffixes in order to make the terms dictionary more space-efficient. Two compression algorithms are used depending on which one is more space-efficient: - LowercaseAsciiCompression, which applies when all bytes are in the `[0x1F,0x3F)` or `[0x5F,0x7F)` ranges, which notably include all digits, lowercase ASCII characters, '.', '-' and '_', and encodes 4 chars on 3 bytes. It is very often applicable on analyzed content and decompresses very quickly thanks to auto-vectorization support in the JVM. - LZ4, when the compression ratio is less than 0.75. I was a bit unhappy with the complexity of the high-compression LZ4 option, so I simplified it in order to only keep the logic that detects duplicate strings. The logic about what to do in case overlapping matches are found, which was responsible for most of the complexity while only yielding tiny benefits, has been removed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361432710
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
Review comment:
A little correction: the **offset** parameter is the start of the match and
the fragmentStart could be different because of the issues described above.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1049: LUCENE-9074: Slice Allocation Circuit Breakers in IndexSearcher
jpountz commented on a change in pull request #1049: LUCENE-9074: Slice
Allocation Circuit Breakers in IndexSearcher
URL: https://github.com/apache/lucene-solr/pull/1049#discussion_r361429190
##
File path:
lucene/core/src/java/org/apache/lucene/search/SliceExecutionControlPlane.java
##
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.util.concurrent.Executor;
+
+/**
+ * Manages the end to end execution and management of slices for an
IndexSearcher
+ * instance. This includes execution on the underlying Executor and managing
+ * thread allocations to ensure a consistent throughput under varying stress
loads.
+ * This class can block allocation of new slices when executing a query
+ * Remaining segments will be allocated to a single slice
+ * NOTE: This can be degrading to query performance since one thread can
+ * be overloaded with multiple segments hence this is a tradeoff
+ * between query throughput and latency
+ *
+ * A typical implementation of this interface would include the execution
framework
+ * along with a circuit breaker condition which will control whether new
threads will be
+ * created or not.
+ */
+public interface SliceExecutionControlPlane {
+
+ /**
+ * Return true if the circuit breaker condition has triggered,
+ * false otherwise
+ */
+ boolean hasCircuitBreakerTriggered();
Review comment:
I was thinking the latter indeed, having more and more slices in a single
task as the number of entries in the queue increases.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified highlighter with word separator never gives context to the left
Traktormaster commented on a change in pull request #1123: LUCENE-9093: Unified
highlighter with word separator never gives context to the left
URL: https://github.com/apache/lucene-solr/pull/1123#discussion_r361426082
##
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/LengthGoalBreakIterator.java
##
@@ -174,7 +175,48 @@ private int moveToBreak(int idx) { // precondition: idx
is a known break
// called at start of new Passage given first word start offset
@Override
public int preceding(int offset) {
-return baseIter.preceding(offset); // no change needed
+final int fragmentStart = Math.max(baseIter.preceding(offset), 0); //
convert DONE to 0
+fragmentEndFromPreceding = baseIter.following(fragmentStart);
Review comment:
Unfortunately no. The fragmentStart argument is the start of the match that
could be anything depending on the tokenizer in the index analyzer chain. Even
if we assume it's the start of a word or a phrase, the underlying BI can break
on different places. In case of SENTENCE the preceding() call here will find
the beginning of the sentence. In case of SEPARATOR, which is customizable by
query, the breaks can be anywhere else.
We could only assume fragmentStart is a break point if the underlying BI
would be the same as the tokenizer in the index analyzer chain. (I'm not sure,
but the query analyzer chain could be different I think.)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14102) FacetModule use number of responses from shards as number of shards
[ https://issues.apache.org/jira/browse/SOLR-14102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003562#comment-17003562 ] Mikhail Khludnev commented on SOLR-14102: - I think we need to assert it somehow via some test. > FacetModule use number of responses from shards as number of shards > --- > > Key: SOLR-14102 > URL: https://issues.apache.org/jira/browse/SOLR-14102 > Project: Solr > Issue Type: Bug > Security Level: Public(Default Security Level. Issues are Public) >Reporter: Tomohiro Manabe >Priority: Major > Attachments: SOLR-14102.patch > > > Hi, I am developing a SearchComponent which sends some requests to only a > part of shards. > Basically it works well, but using it with facetting triggers an error. > I found that FacetModule use the number of responses from shards > (sreq.responses.size()) as the number of shards. > Of course this assumption is incorrect in case that only a part of shards > respond. > Instead of the number of responses, using rb.shards.length may be better. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14151) Make schema components load from packages
[
https://issues.apache.org/jira/browse/SOLR-14151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003552#comment-17003552
]
Noble Paul commented on SOLR-14151:
---
I can't comment on SPI now. All I can say is that this ticket just makes
plugins in schema.xml will behave exactly like the ones in solrconfig.xml.
IMHO , the discussion on whether to have SPI support is orthogonal
> Make schema components load from packages
> -
>
> Key: SOLR-14151
> URL: https://issues.apache.org/jira/browse/SOLR-14151
> Project: Solr
> Issue Type: Sub-task
> Security Level: Public(Default Security Level. Issues are Public)
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Example:
> {code:xml}
>
>
>
>generateNumberParts="0" catenateWords="0"
> catenateNumbers="0" catenateAll="0"/>
>
>
>
>
> {code}
> * When a package is updated, the entire {{IndexSchema}} object is refreshed,
> but the SolrCore object is not reloaded
> * Any component can be prefixed with the package name
> * The semantics of loading plugins remain the same as that of the components
> in {{solrconfig.xml}}
> * Plugins can be registered using schema API
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14151) Make schema components load from packages
[
https://issues.apache.org/jira/browse/SOLR-14151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003545#comment-17003545
]
Jan Høydahl commented on SOLR-14151:
Please clarify how this relates to the style of
analyzer loading, if spi loading will be supported from packages and if so with
what syntax. If not supported, that should also be documented.
> Make schema components load from packages
> -
>
> Key: SOLR-14151
> URL: https://issues.apache.org/jira/browse/SOLR-14151
> Project: Solr
> Issue Type: Sub-task
> Security Level: Public(Default Security Level. Issues are Public)
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Example:
> {code:xml}
>
>
>
>generateNumberParts="0" catenateWords="0"
> catenateNumbers="0" catenateAll="0"/>
>
>
>
>
> {code}
> * When a package is updated, the entire {{IndexSchema}} object is refreshed,
> but the SolrCore object is not reloaded
> * Any component can be prefixed with the package name
> * The semantics of loading plugins remain the same as that of the components
> in {{solrconfig.xml}}
> * Plugins can be registered using schema API
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[jira] [Commented] (SOLR-14102) FacetModule use number of responses from shards as number of shards
[
https://issues.apache.org/jira/browse/SOLR-14102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17003543#comment-17003543
]
Lucene/Solr QA commented on SOLR-14102:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
|| || || || {color:brown} Prechecks {color} ||
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m
0s{color} | {color:red} The patch doesn't appear to include any new or modified
tests. Please justify why no new tests are needed for this patch. Also please
list what manual steps were performed to verify this patch. {color} |
|| || || || {color:brown} master Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
1s{color} | {color:green} master passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Release audit (RAT) {color} |
{color:green} 1m 0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Check forbidden APIs {color} |
{color:green} 1m 0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Validate source patterns {color} |
{color:green} 1m 0s{color} | {color:green} the patch passed {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 46m
19s{color} | {color:green} core in the patch passed. {color} |
| {color:black}{color} | {color:black} {color} | {color:black} 50m 6s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | SOLR-14102 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12989450/SOLR-14102.patch |
| Optional Tests | compile javac unit ratsources checkforbiddenapis
validatesourcepatterns |
| uname | Linux lucene1-us-west 4.15.0-54-generic #58-Ubuntu SMP Mon Jun 24
10:55:24 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | ant |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-SOLR-Build/sourcedir/dev-tools/test-patch/lucene-solr-yetus-personality.sh
|
| git revision | master / 7350f03cd10 |
| ant | version: Apache Ant(TM) version 1.10.5 compiled on March 28 2019 |
| Default Java | LTS |
| Test Results |
https://builds.apache.org/job/PreCommit-SOLR-Build/639/testReport/ |
| modules | C: solr/core U: solr/core |
| Console output |
https://builds.apache.org/job/PreCommit-SOLR-Build/639/console |
| Powered by | Apache Yetus 0.7.0 http://yetus.apache.org |
This message was automatically generated.
> FacetModule use number of responses from shards as number of shards
> ---
>
> Key: SOLR-14102
> URL: https://issues.apache.org/jira/browse/SOLR-14102
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
>Reporter: Tomohiro Manabe
>Priority: Major
> Attachments: SOLR-14102.patch
>
>
> Hi, I am developing a SearchComponent which sends some requests to only a
> part of shards.
> Basically it works well, but using it with facetting triggers an error.
> I found that FacetModule use the number of responses from shards
> (sreq.responses.size()) as the number of shards.
> Of course this assumption is incorrect in case that only a part of shards
> respond.
> Instead of the number of responses, using rb.shards.length may be better.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361408293 ## File path: solr/core/src/java/org/apache/solr/core/SolrCore.java ## @@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo MDCLoggingContext.setCore(this); resourceLoader = config.getResourceLoader(); + resourceLoader.core = this; Review comment: Is this safe? Isn’t SRL shared by many cores? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361408428
##
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map attrs,
NamedList initArgs, Li
/** class names can be prefixed with package name e.g:
my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair parseClassName(String name) {
-String pkgName = null;
-String className = name;
-if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
-pkgName = name.substring(0, colonIdx);
-className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
Find a better name for this class?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema
components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361411570
##
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f ->
PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
Why the instanceof check here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569014248 > RE "inconsistent state": Put differently, I think you are just pointing out that this PR is not loading the correct version? Yes; this is a known blocker/nocommit. Did I mention this was a hackday project ;-) > > The latter part of your message seems to point to an as-yet unimplemented improvement proposal for the package management system (is this the right name?) to track which plugin abstractions are hot-loadable or not. I don't think that's in scope of this PR. BTW so that we don't confuse each other, I propose that "hot-loadable" imply _no_ core reload. We should document clearly, which plugins are - reloaded without reloading core - which are hot loaded - or somethings cannot even be reloaded and it will fail Hot reloading is not important. But clearly documenting and setting expectations is important I clearly understand this is a hackday PR. I have no complaints about it. You seem to have misconstrued by feedback as a criticism. I just wrote down my observations. (Some observations may even be wrong as I wouldn't have thoroughly gone through the huge PR). We definitely want more review comments on our PRs and not less. So, let's make life a bit easier for the reviewers. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul removed a comment on issue #1109: More pervasive use of PackageLoader / PluginInfo
noblepaul removed a comment on issue #1109: More pervasive use of PackageLoader / PluginInfo URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-568981311 @dsmiley The comments are not supposed to be personal. These are my observations on the code/PR. I'm sure somebody is going to remove that "WIP" tag pretty soon. There is a ton of work that needs to be done before we can do that. So, I'm typing down my observations as I see them. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
noblepaul commented on a change in pull request #1109: More pervasive use of
PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361401294
##
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig
config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className,
DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info,
DirectoryFactory.class);
Review comment:
yeah, schema is shared. SolrConfig is not. Maybe it's safe
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets
mkhludnev commented on a change in pull request #1122: SOLR-12490 Added parsing
of json.queries for referring in JSON facets
URL: https://github.com/apache/lucene-solr/pull/1122#discussion_r361400807
##
File path:
solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java
##
@@ -28,6 +28,10 @@
* @lucene.internal
*/
class JsonQueryConverter {
+ public static final String paramsPrefix = "_tt";
+
+ public static final Object contextKey =
JsonQueryConverter.class.getSimpleName();
Review comment:
Now, It's redundant, I suppose.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets
mkhludnev commented on a change in pull request #1122: SOLR-12490 Added parsing
of json.queries for referring in JSON facets
URL: https://github.com/apache/lucene-solr/pull/1122#discussion_r361401011
##
File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java
##
@@ -214,48 +215,60 @@ public static void processParams(SolrRequestHandler
handler, SolrQueryRequest re
out = "rows";
} else if (SORT.equals(key)) {
out = SORT;
+} else if ("queries".equals(key)) {
+ Object queriesJsonObj = entry.getValue();
+ if (queriesJsonObj instanceof Map) {
+for (Map.Entry queryJsonProperty : ((Map) queriesJsonObj).entrySet()) {
+ out = queryJsonProperty.getKey();
+ arr = true;
+ isQuery = true;
+ convertJsonPropertyToLocalParams(newMap, jsonQueryConverter,
queryJsonProperty, out, isQuery, arr);
+}
+continue;
+ } else {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
"Expected Map for 'queries', received " +
queriesJsonObj.getClass().getSimpleName() + "=" + queriesJsonObj);
Review comment:
not Map. but Object for json.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
noblepaul commented on a change in pull request #1109: More pervasive use of
PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361375529
##
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String
resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className,
IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info,
IndexSchemaFactory.class);
Review comment:
> There is no right/wrong here; we are exploring what the future holds
together; it has not been written yet with no spec to judge right/wrong
"SRL has no idea which version of package should be loaded".
This was my statement. I never said SRL loading classes is "plain wrong".
According to the PR it was loading the latest version of the package and "that
is wrong". Am I supposed to comment on what the PR will look like in the future
or am I supposed to make comments on what it does today?
I think you are working during the holidays and you are not reading the
comments fully
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets
a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added
parsing of json.queries for referring in JSON facets
URL: https://github.com/apache/lucene-solr/pull/1122#discussion_r361398204
##
File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java
##
@@ -214,37 +216,22 @@ public static void processParams(SolrRequestHandler
handler, SolrQueryRequest re
out = "rows";
} else if (SORT.equals(key)) {
out = SORT;
+} else if ("queries".equals(key)) {
+ for (Map.Entry subEntry : ((Map)
entry.getValue()).entrySet()) {
+out = subEntry.getKey();
+arr = true;
+isQuery = true;
+processJsonEntry(newMap, jsonQueryConverter, subEntry, out,
isQuery, arr);
+ }
+ continue;
} else if ("params".equals(key) || "facet".equals(key) ) {
// handled elsewhere
continue;
} else {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
"Unknown top-level key in JSON request : " + key);
}
-Object val = entry.getValue();
-
-if (arr) {
- String[] existing = newMap.get(out);
- List lst = val instanceof List ? (List)val : null;
- int existingSize = existing==null ? 0 : existing.length;
- int jsonSize = lst==null ? 1 : lst.size();
- String[] newval = new String[ existingSize + jsonSize ];
- for (int i=0; i
[GitHub] [lucene-solr] a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets
a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added
parsing of json.queries for referring in JSON facets
URL: https://github.com/apache/lucene-solr/pull/1122#discussion_r361398218
##
File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java
##
@@ -214,37 +216,22 @@ public static void processParams(SolrRequestHandler
handler, SolrQueryRequest re
out = "rows";
} else if (SORT.equals(key)) {
out = SORT;
+} else if ("queries".equals(key)) {
+ for (Map.Entry subEntry : ((Map)
entry.getValue()).entrySet()) {
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets
a-siuniaev commented on a change in pull request #1122: SOLR-12490 Added parsing of json.queries for referring in JSON facets URL: https://github.com/apache/lucene-solr/pull/1122#discussion_r361397810 ## File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java ## @@ -191,7 +191,9 @@ public static void processParams(SolrRequestHandler handler, SolrQueryRequest re } // implement compat for existing components... -JsonQueryConverter jsonQueryConverter = new JsonQueryConverter(); +JsonQueryConverter jsonQueryConverter = (JsonQueryConverter) req.getContext() Review comment: True. Reversed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
