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 2022/05/17 18:37:54 UTC
[GitHub] [lucene] shahrs87 opened a new pull request, #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
shahrs87 opened a new pull request, #898:
URL: https://github.com/apache/lucene/pull/898
<!--
_(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:
* https://issues.apache.org/jira/projects/LUCENE
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>
LUCENE 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://github.com/apache/lucene/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have given Lucene 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 `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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-unsubscribe@lucene.apache.org
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] dsmiley closed pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
dsmiley closed pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
URL: https://github.com/apache/lucene/pull/898
--
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-unsubscribe@lucene.apache.org
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] shahrs87 commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r876075120
##########
lucene/CHANGES.txt:
##########
@@ -40,7 +40,7 @@ Improvements
Optimizations
---------------------
-(No changes)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
Just to understand the process, I will have to create 2 PR's, one for `main` branch and other for `branch_9x`, correct ? @mikemccand @dsmiley @romseygeek
--
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-unsubscribe@lucene.apache.org
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] dsmiley commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875264422
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
But this is an Optimization (should thus go right below); no?
--
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-unsubscribe@lucene.apache.org
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] romseygeek commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
romseygeek commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875631623
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
I've created the 9.2 branch, so feel free to backport this to 9.x and put it in the 9.3 CHANGES section.
--
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-unsubscribe@lucene.apache.org
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] dsmiley commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r876148753
##########
lucene/CHANGES.txt:
##########
@@ -40,7 +40,7 @@ Improvements
Optimizations
---------------------
-(No changes)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
As a contributor, you can just concern yourself with main. After merging your PR to main, I'll do a back-port to 9x. If it's non-trivial, I'll submit a 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-unsubscribe@lucene.apache.org
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] shahrs87 commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r876161864
##########
lucene/CHANGES.txt:
##########
@@ -40,7 +40,7 @@ Improvements
Optimizations
---------------------
-(No changes)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
@dsmiley Hopefully now I got the changes right. Thank you for your patience. :)
--
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-unsubscribe@lucene.apache.org
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] shahrs87 commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875273788
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
I am pretty new to this project. This is my 2nd commit. So I don't know much about the release versions. Just that I understand clearly, we will wait for couple of weeks and once 9.2 is released and 9.3's section is created, I need to change CHANGES.txt and then we will merge 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-unsubscribe@lucene.apache.org
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] shahrs87 commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r876089065
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
Just to understand the process, I will have to create 2 PR's, one for main branch and other for branch_9x, correct ?
--
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-unsubscribe@lucene.apache.org
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] mikemccand commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
mikemccand commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875951319
##########
lucene/CHANGES.txt:
##########
@@ -40,7 +40,7 @@ Improvements
Optimizations
---------------------
-(No changes)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
Since 9.2 release branch is cut, if you re-base, you'll see a new empty 9.3.0 section in `CHANGES.txt` and you can add your entry there. It'll be the first one, yay!
--
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-unsubscribe@lucene.apache.org
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] mikemccand commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
mikemccand commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875952557
##########
lucene/core/src/java/org/apache/lucene/index/MultiDocValues.java:
##########
@@ -53,8 +53,18 @@ public static NumericDocValues getNormValues(final IndexReader r, final String f
} else if (size == 1) {
return leaves.get(0).reader().getNormValues(field);
}
- FieldInfo fi = FieldInfos.getMergedFieldInfos(r).fieldInfo(field); // TODO avoid merging
- if (fi == null || fi.hasNorms() == false) {
+
+ // Check if any of the leaf reader which has this field has norms.
+ boolean normFound = false;
+ for (LeafReaderContext leaf : leaves) {
+ LeafReader reader = leaf.reader();
+ FieldInfo info = reader.getFieldInfos().fieldInfo(field);
+ if (info != null && info.hasNorms()) {
+ normFound = true;
+ break;
+ }
+ }
+ if (!normFound) {
Review Comment:
Maybe use `normFound == false` instead? (For better readability and to reduce the risk of future refactoring bugs).
--
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-unsubscribe@lucene.apache.org
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] dsmiley commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875271168
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
And you've put this in the 10.0 changes but I see no reason not to backport to 9.x. There's a feature-freeze for 9.2 (it's going to be released) so... we could just wait a week or two here for the 9.3 section to appear by @romseygeek (the RM).
--
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-unsubscribe@lucene.apache.org
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] shahrs87 commented on pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #898:
URL: https://github.com/apache/lucene/pull/898#issuecomment-1129196482
Hi @dsmiley
Can you please help me review this patch ? I have tried to implement this using your suggestion in the jira. Thank you.
--
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-unsubscribe@lucene.apache.org
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] shahrs87 commented on a diff in pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on code in PR #898:
URL: https://github.com/apache/lucene/pull/898#discussion_r875270812
##########
lucene/CHANGES.txt:
##########
@@ -38,6 +38,8 @@ Improvements
* LUCENE-10416: Update Korean Dictionary to mecab-ko-dic-2.1.1-20180720 for Nori.
(Uihyun Kim)
+* LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos (Rushabh Shah)
Review Comment:
True. Changed it in latest commit. Please review again.
--
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-unsubscribe@lucene.apache.org
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] dsmiley commented on pull request #898: LUCENE-8519 MultiDocValues.getNormValues should not call getMergedFieldInfos
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #898:
URL: https://github.com/apache/lucene/pull/898#issuecomment-1133815329
I ended up cherry picking #902 so I'll close 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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
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