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/02/24 12:30:22 UTC
[GitHub] [lucene-solr] thomaswoeckinger opened a new pull request #2426: SOLR-15191: Fix faceting on EnumFieldType
thomaswoeckinger opened a new pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene or Solr:
* https://issues.apache.org/jira/projects/LUCENE
* https://issues.apache.org/jira/projects/SOLR
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
* SOLR-####: <short description of problem or changes>
LUCENE and SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `master` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-786456852
> Thanks; looks good to me and is simpler too. I ran tests. Can you propose a CHANGES.txt entry please? I suppose one aspect of this is a bug as indicated in your JIRA text (so it goes there), but maybe also list this under improvements (or optimizations?) saying something like: the hash method of json faceting now supports enum field types and perhaps some custom ones too.
>
Is 'David Smiley, Thomas Wöckinger' sufficent as authors at the end of the line?
> I normally go from master and cherry pick to 8x but I suppose there is no harm in going the other way here.
I know, this is due to the fact i have not started development against 9.x at the moment, this is on the road map next month
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-786091771
Maybe I'm missing something but my suggestion is (in my head) very simple. Remove the line `if (ft instanceof TrieField || ft.isPointField())` and closing brace for it, and perhaps also the "else" side. The `default` case of the switch I think should handle the null. If this won't work then why not?
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-785630714
Instead of the instanceof checks (kinda brittle code-smell, not flexible), maybe we can call `FieldType.getNumberType() != null` ?
Can you please run all tests, e.g. `/gradlew check`
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-786409516
Thanks; looks good to me and is simpler too. I ran tests. Can you propose a CHANGES.txt entry please? I suppose one aspect of this is a bug as indicated in your JIRA text (so it goes there), but maybe also list this under improvements (or optimizations?) saying something like: the hash method of json faceting now supports enum field types and perhaps some custom ones too.
I normally go from master and cherry pick to 8x but I suppose there is no harm in going the other way 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:
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-785663364
> Instead of the instanceof checks (kinda brittle code-smell, not flexible), maybe we can call `FieldType.getNumberType() != null` ?
>
That is already done in FacetFieldProcessorByHashDV.calcFacets which is the only position which is using this method, and i agree with you it's kind of a code smell.
There are two alternatives (i don't really like one of them):
a.) Move the getNumberType()!=null check into the method but than i have to catch the exception outside or breaking the behavior.
b.) Copy only the switch case statement to FacetFieldProcessorByHashDV.calcFacets.
> Can you please run all tests, e.g. `/gradlew check`
I already run ant test, there are no test failures.
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger edited a comment on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-785663364
> Instead of the instanceof checks (kinda brittle code-smell, not flexible), maybe we can call `FieldType.getNumberType() != null` ?
>
That is already done in FacetFieldProcessorByHashDV.calcFacets which is the only position which is using this method, and i agree with you it's kind of a code smell.
There are two alternatives (i don't really like one of them):
a.) Move the getNumberType()!=null check into the method but than i have to catch the exception outside or breaking the behavior.
b.) Copy only the switch case statement to FacetFieldProcessorByHashDV.calcFacets.
> Can you please run all tests, e.g. `/gradlew check`
I already run ant test, there are no test failures.
----------------------------------------------------------------
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 #2426: SOLR-15191: Fix faceting on EnumFieldType
Posted by GitBox <gi...@apache.org>.
thomaswoeckinger commented on pull request #2426:
URL: https://github.com/apache/lucene-solr/pull/2426#issuecomment-786156607
> Maybe I'm missing something but my suggestion is (in my head) very simple. Remove the line `if (ft instanceof TrieField || ft.isPointField())` and closing brace for it, and perhaps also the "else" side. The `default` case of the switch I think should handle the null. If this won't work then why not?
I fully understand your suggestion, the first PR was done with minimal changes only to avoid side effects.
I pushed again and refactored the method a bit more, just a note switch can not handle null in java, anyway have look.
----------------------------------------------------------------
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