Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley merged PR #3158: URL: https://github.com/apache/solr/pull/3158 -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1948247603
##
solr/core/src/java/org/apache/solr/search/facet/FieldUtil.java:
##
@@ -44,27 +48,49 @@ public static SortedDocValues
getSortedDocValues(SolrIndexSearcher searcher, Str
public static SortedDocValues getSortedDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SortedDocValues si =
-
context.searcher().getSlowAtomicReader().getSortedDocValues(field.getName());
-// if (!field.hasDocValues() && (field.getType() instanceof StrField ||
field.getType()
-// instanceof TextField)) {
-// }
-
-return si == null ? DocValues.emptySorted() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getSortedDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptySorted() : dv;
}
public static SortedSetDocValues getSortedSetDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SortedSetDocValues si =
-
context.searcher().getSlowAtomicReader().getSortedSetDocValues(field.getName());
-return si == null ? DocValues.emptySortedSet() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getSortedSetDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptySortedSet() : dv;
}
public static NumericDocValues getNumericDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SolrIndexSearcher searcher = context.searcher();
-NumericDocValues si =
searcher.getSlowAtomicReader().getNumericDocValues(field.getName());
-return si == null ? DocValues.emptyNumeric() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getNumericDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptyNumeric() : dv;
+ }
+
+ private static void checkDvType(Object dv, SchemaField field, LeafReader
reader) {
Review Comment:
There's wasn't a class cast; there was empty/no data which ultimately leads
to no facet results. That's *worse* than throwing an exception (of whatever
type; no strong opinion from me). This is my primary addition to the PR; don't
*just* fix the wrong algorithm choice, but ensure we throw an exception in
cases that would previously silently produce nothing.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1948246532
##
solr/core/src/java/org/apache/solr/search/facet/FacetField.java:
##
@@ -139,11 +139,11 @@ public FacetProcessor
createFacetProcessor(FacetContext fcontext) {
// FieldType.getDocValuesType()
if (!multiToken) {
- if (mincount > 0 && prefix == null && (ntype != null || method ==
FacetMethod.DVHASH)) {
+ if (mincount > 0 && prefix == null && (isNumber || method ==
FacetMethod.DVHASH)) {
// TODO can we auto-pick for strings when term cardinality is much
greater than DocSet
// cardinality? or if we don't know cardinality but DocSet size is
very small
return new FacetFieldProcessorByHashDV(fcontext, this, sf);
- } else if (ntype == null) {
+ } else if (isNumber == false) {
Review Comment:
Maybe you are not familiar that Lucene & Solr has recognized this style as
acceptable due to its clarity
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
mkhludnev commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1948227521
##
solr/core/src/java/org/apache/solr/search/facet/FacetField.java:
##
@@ -139,11 +139,11 @@ public FacetProcessor
createFacetProcessor(FacetContext fcontext) {
// FieldType.getDocValuesType()
if (!multiToken) {
- if (mincount > 0 && prefix == null && (ntype != null || method ==
FacetMethod.DVHASH)) {
+ if (mincount > 0 && prefix == null && (isNumber || method ==
FacetMethod.DVHASH)) {
// TODO can we auto-pick for strings when term cardinality is much
greater than DocSet
// cardinality? or if we don't know cardinality but DocSet size is
very small
return new FacetFieldProcessorByHashDV(fcontext, this, sf);
- } else if (ntype == null) {
+ } else if (isNumber == false) {
Review Comment:
!isNumber
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
mkhludnev commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1948229222
##
solr/core/src/java/org/apache/solr/search/facet/FieldUtil.java:
##
@@ -44,27 +48,49 @@ public static SortedDocValues
getSortedDocValues(SolrIndexSearcher searcher, Str
public static SortedDocValues getSortedDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SortedDocValues si =
-
context.searcher().getSlowAtomicReader().getSortedDocValues(field.getName());
-// if (!field.hasDocValues() && (field.getType() instanceof StrField ||
field.getType()
-// instanceof TextField)) {
-// }
-
-return si == null ? DocValues.emptySorted() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getSortedDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptySorted() : dv;
}
public static SortedSetDocValues getSortedSetDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SortedSetDocValues si =
-
context.searcher().getSlowAtomicReader().getSortedSetDocValues(field.getName());
-return si == null ? DocValues.emptySortedSet() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getSortedSetDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptySortedSet() : dv;
}
public static NumericDocValues getNumericDocValues(
QueryContext context, SchemaField field, QParser qparser) throws
IOException {
-SolrIndexSearcher searcher = context.searcher();
-NumericDocValues si =
searcher.getSlowAtomicReader().getNumericDocValues(field.getName());
-return si == null ? DocValues.emptyNumeric() : si;
+var reader = context.searcher().getSlowAtomicReader();
+var dv = reader.getNumericDocValues(field.getName());
+checkDvType(dv, field, reader);
+return dv == null ? DocValues.emptyNumeric() : dv;
+ }
+
+ private static void checkDvType(Object dv, SchemaField field, LeafReader
reader) {
Review Comment:
It's not clear why to have this 20 lines. If we remove it, I think it just
fails at class cast somewhere later. Why ISE is better than ClassCastException
later? It's just not obvious.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on PR #3158: URL: https://github.com/apache/solr/pull/3158#issuecomment-2646477023 Looking for additional code review since I contributed to this PR substantially. -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on code in PR #3158: URL: https://github.com/apache/solr/pull/3158#discussion_r1945285244 ## solr/CHANGES.txt: ## @@ -187,6 +187,8 @@ Bug Fixes * SOLR-17652: Fix a bug that could cause long leader elections to leave PULL replicas in DOWN state forever. (hossman) +* SOLR-17649: Fix a regression of faceting on multi-valued EnumFieldType, introduced by an earlier 9.x version. (Thomas Wöckinger, David Smiley, Houston Putman, Kevin Risden) Review Comment: LOL if I merge this I'll just list yourself; I don't want any hint of credit. As a reviewer my input was too trivial in this case to justify my name being listed. -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1944771535
##
solr/core/src/java/org/apache/solr/search/facet/FacetField.java:
##
@@ -152,7 +152,7 @@ public FacetProcessor
createFacetProcessor(FacetContext fcontext) {
}
}
Review Comment:
There's a comment below: `// multi-valued after this point`. Can you please
move that up above the condition you are modifying, since I think it belongs
higher.
##
solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java:
##
@@ -4907,6 +4907,38 @@ public void testQueryJoinBooksAndPages() throws
Exception {
+ ", books2:{ buckets:[ {val:q,count:1}, {val:w,count:1} ] }"
+ "}");
}
+
+ @Test
+ public void testMultivalueEnumTypes() throws Exception {
+final Client client = Client.localClient();
+
+final SolrParams p = params("rows", "0");
+
+client.deleteByQuery("*:*", null);
+
+List docsToAdd = new ArrayList<>(6);
Review Comment:
Whoops; right; never mind. We have too many weird test infrastructure
things.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
thomaswoeckinger commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1944322475
##
solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java:
##
@@ -4907,6 +4907,38 @@ public void testQueryJoinBooksAndPages() throws
Exception {
+ ", books2:{ buckets:[ {val:q,count:1}, {val:w,count:1} ] }"
+ "}");
}
+
+ @Test
+ public void testMultivalueEnumTypes() throws Exception {
+final Client client = Client.localClient();
+
+final SolrParams p = params("rows", "0");
+
+client.deleteByQuery("*:*", null);
+
+List docsToAdd = new ArrayList<>(6);
Review Comment:
I tried but this will make the test more complicated as
SolrTestCaseHS.Client does not keep a SolrClient and uses null therefore in
internal API calls, may you have an hint for that, but the whole test class is
not using an UpdateRequest
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
thomaswoeckinger commented on PR #3158: URL: https://github.com/apache/solr/pull/3158#issuecomment-2639051999 > It's unfortunate we can't check the docValues format, but can we at least add a comment explaining this. It's a bit confusing just looking at it without context. FacetFieldProcessorByArrayDV does not handle DocValuesType.SORTED_NUMERIC because in method findStartAndEndOrds FieldUtil.getSortedSetDocValues is used, which can not handle this kind of DocValuesType. In detail FacetFieldProcessorByArrayDV extends FacetFieldProcessorByArray which seems to be designed for DocValuesType.SORTED_SET, because the method lookupOrd is only possible when having such a type. Saying that, you may have some suggestions for a code comment. For me it seems this was not working form the beginning of the 9x branch, there was simply not test for it. -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
dsmiley commented on code in PR #3158:
URL: https://github.com/apache/solr/pull/3158#discussion_r1943824313
##
solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java:
##
@@ -4907,6 +4907,38 @@ public void testQueryJoinBooksAndPages() throws
Exception {
+ ", books2:{ buckets:[ {val:q,count:1}, {val:w,count:1} ] }"
+ "}");
}
+
+ @Test
+ public void testMultivalueEnumTypes() throws Exception {
+final Client client = Client.localClient();
+
+final SolrParams p = params("rows", "0");
+
+client.deleteByQuery("*:*", null);
+
+List docsToAdd = new ArrayList<>(6);
Review Comment:
Can you add them to an UpdateRequest so they are batched instead of sending
them individually? Same LOC in the end but avoids an anti-pattern. Or is the
separation pertinent to what's being tested?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
thomaswoeckinger commented on PR #3158: URL: https://github.com/apache/solr/pull/3158#issuecomment-2636198561 @dsmiley @gerlowskija May you know who is responsible for this part of faceting code, to get a reviewer -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17649: Fix Json faceting on multivalue number types [solr]
thomaswoeckinger commented on PR #3158: URL: https://github.com/apache/solr/pull/3158#issuecomment-2634739177 @risdenk as your are one of the last commiters in this area, you may have a look at this PR -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
