You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/06/08 07:49:09 UTC
[GitHub] [lucene-solr] thomaswoeckinger opened a new pull request #2508: SOLR-15457: Returning EnvmFieldValue in faceting buckets again.
thomaswoeckinger opened a new pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647825585
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
Review comment:
I already added an additional request to work directly on the objects and check the instanceof EnumFieldValue
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#issuecomment-856795094
> If some of those methods are only used in range faceting, perhaps it doesn't make sense to support range faceting on an enum field any way? Thus it'd be okay to throw as being unsupported.
I have to check 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647648735
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
Review comment:
Added test to verify correct instance of results
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647383022
##########
File path: solr/core/src/test/org/apache/solr/schema/EnumFieldTest.java
##########
@@ -592,6 +592,11 @@ public void testFacetEnumSearch() throws Exception {
assertQ(req("fl", "" + FIELD_NAME, "q", FIELD_NAME + ":*", "json.facet", jsonFacetParam),
"//*[@name='facets']/int/text()=6",
- "//*[@name='allBuckets']/long/text()=6");
+ "//*[@name='allBuckets']/long/text()=6",
Review comment:
So there was a gap in our testing then, I suppose?
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
Review comment:
The test didn't exercise this. The impl looks suspicious because it parses a String to an integer.
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
+ Integer val = Integer.valueOf(rawval);
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, String gap) {
Review comment:
The test didn't exercise this. The impl looks suspicious because it parses a String to an integer.
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -171,22 +176,7 @@ public static Calc getNumericCalc(SchemaField sf) {
private static Calc getCalcForField(SchemaField sf) {
final FieldType ft = sf.getType();
if (ft instanceof TrieField || ft.isPointField()) {
- switch (ft.getNumberType()) {
- case FLOAT:
- return new FloatCalc(sf);
- case DOUBLE:
- return new DoubleCalc(sf);
- case INTEGER:
- return new IntCalc(sf);
- case LONG:
- return new LongCalc(sf);
- case DATE:
- return new DateCalc(sf, null);
- default:
- throw new SolrException
- (SolrException.ErrorCode.BAD_REQUEST,
- "Unable to range facet on numeric field of unexpected type:" + sf.getName());
- }
+ return getNumericCalc(sf);
Review comment:
Nice!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley merged pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#issuecomment-857351201
I don't know what the point is in reaching into the response type to do an instanceof check. Testing the values are returned and not ordinals is the real important part. Any way I committed this as-is. Thank you for contributing!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#issuecomment-856774500
If some of those methods are only used in range faceting, perhaps it doesn't make sense to support range faceting on an enum field any way? Thus it'd be okay to throw as being unsupported.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger edited a comment on pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger edited a comment on pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#issuecomment-856900350
> If some of those methods are only used in range faceting, perhaps it doesn't make sense to support range faceting on an enum field any way? Thus it'd be okay to throw as being unsupported.
I checked the call hierarchy and both methods (parseStr, parseAndAddGap) are only used by FacetRangeProcessor.
Regarding range faceting: Is it impossible to use enum values?
Because they have a ordinal associated with their value, anyway i will change it to throw an appropriate exception.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647471384
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
Review comment:
It is a bit difficult to test it directly, the respone contains EnumFieldValue instances which are convert to to the string value by writing out the response as XML by TestHarness query response writer. Before the change the int values are written out as string, so the test at least test a part of the expected behavior
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#issuecomment-856900350
> If some of those methods are only used in range faceting, perhaps it doesn't make sense to support range faceting on an enum field any way? Thus it'd be okay to throw as being unsupported.
I checked the call hierarchy and both methods (parseStr, parseAndAddGap) are only used by FacetRangeProcessor.
Regarding range faceting: Is it impossible to use enum values?
Because they have a ordinal associated with their 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] thomaswoeckinger commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647459338
##########
File path: solr/core/src/test/org/apache/solr/schema/EnumFieldTest.java
##########
@@ -592,6 +592,11 @@ public void testFacetEnumSearch() throws Exception {
assertQ(req("fl", "" + FIELD_NAME, "q", FIELD_NAME + ":*", "json.facet", jsonFacetParam),
"//*[@name='facets']/int/text()=6",
- "//*[@name='allBuckets']/long/text()=6");
+ "//*[@name='allBuckets']/long/text()=6",
Review comment:
Yes, found by a new integration test on our side
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2508: SOLR-15457: Returning EnumFieldValue in faceting buckets again.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2508:
URL: https://github.com/apache/lucene-solr/pull/2508#discussion_r647761533
##########
File path: solr/core/src/java/org/apache/solr/search/facet/FacetRangeProcessor.java
##########
@@ -774,6 +763,30 @@ public Long parseAndAddGap(@SuppressWarnings("rawtypes") Comparable value, Strin
}
}
+ private static class EnumCalc extends Calc {
+
+ private final EnumMapping mapping;
+ public EnumCalc(final SchemaField f) {
+ super(f);
+ mapping = ((AbstractEnumField)field.getType()).getEnumMapping();
+ }
+ @Override
+ public EnumFieldValue bitsToValue(long bits) {
+ Integer val = (int)bits;
+ return new EnumFieldValue(val, mapping.intValueToStringValue(val));
+ }
+ @Override
+ protected EnumFieldValue parseStr(String rawval) {
Review comment:
I'm missing the point. I had set a breakpoint but it didn't hit, which caused me to write what I wrote. The test uses XML (because of the XPath based asertions) which is fairly realistic. Would another format perturb the bug?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org