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/09/20 20:30:33 UTC

[GitHub] [lucene] bruno-roustant opened a new pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

bruno-roustant opened a new pull request #311:
URL: https://github.com/apache/lucene/pull/311


   I separated the changes in several commits to be able to drop a commit if it is not appropriate.
   - PerFieldDocValuesFormat and PerFieldPostingsFormat.
   - WDFFactory, WDGFFactory and PerFieldKnnVectorsFormat.
   - Japanese UserDictionary.


-- 
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] bruno-roustant commented on a change in pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #311:
URL: https://github.com/apache/lucene/pull/311#discussion_r712893122



##########
File path: lucene/core/src/java/org/apache/lucene/codecs/perfield/PerFieldPostingsFormat.java
##########
@@ -340,9 +342,15 @@ public FieldsReader(final SegmentReadState readState) throws IOException {
       this.segment = readState.segmentInfo.name;
     }
 
+    private static List<String> buildFieldList(Map<String, FieldsProducer> fields) {
+      List<String> fieldList = new ArrayList<>(fields.keySet());
+      fieldList.sort(null);
+      return Collections.unmodifiableList(fieldList);
+    }

Review comment:
       Thanks, it's clearer indeed.




-- 
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] bruno-roustant commented on pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #311:
URL: https://github.com/apache/lucene/pull/311#issuecomment-933206008


   Ok, I'm closing this PR and the associated JIRA issue as HashMap + List is too complicated and TreeMap was a deliberated choice for keeping the memory usage low.


-- 
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] bruno-roustant closed pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

Posted by GitBox <gi...@apache.org>.
bruno-roustant closed pull request #311:
URL: https://github.com/apache/lucene/pull/311


   


-- 
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] jpountz commented on a change in pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #311:
URL: https://github.com/apache/lucene/pull/311#discussion_r712832329



##########
File path: lucene/core/src/java/org/apache/lucene/codecs/perfield/PerFieldPostingsFormat.java
##########
@@ -340,9 +342,15 @@ public FieldsReader(final SegmentReadState readState) throws IOException {
       this.segment = readState.segmentInfo.name;
     }
 
+    private static List<String> buildFieldList(Map<String, FieldsProducer> fields) {
+      List<String> fieldList = new ArrayList<>(fields.keySet());
+      fieldList.sort(null);
+      return Collections.unmodifiableList(fieldList);
+    }

Review comment:
       nit: `buildFieldList` is a bit abstract and if I was reading a call site of this method I'd need to look at its definition to understand what it does exactly. Maybe call it `createSortedCopy(Collection<String>)` instead and fix call sites to pass the keyset to make the code easier to read?




-- 
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] rmuir commented on a change in pull request #311: LUCENE-10097: Replace TreeMap use by HashMap when unnecessary

Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #311:
URL: https://github.com/apache/lucene/pull/311#discussion_r713746834



##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/miscellaneous/WordDelimiterGraphFilterFactory.java
##########
@@ -156,14 +156,16 @@ public TokenFilter create(TokenStream input) {
             "Invalid Mapping Rule : [" + rule + "]. Only a single character is allowed.");
       if (rhs == null)
         throw new IllegalArgumentException("Invalid Mapping Rule : [" + rule + "]. Illegal type.");
-      typeMap.put(lhs.charAt(0), rhs);
+      char c = lhs.charAt(0);
+      typeMap.put(c, rhs);
+      if (c > maxChar) {
+        maxChar = c;

Review comment:
       Similar question above, why are we adding this code complexity?

##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/miscellaneous/WordDelimiterFilterFactory.java
##########
@@ -169,14 +169,16 @@ public TokenFilter create(TokenStream input) {
             "Invalid Mapping Rule : [" + rule + "]. Only a single character is allowed.");
       if (rhs == null)
         throw new IllegalArgumentException("Invalid Mapping Rule : [" + rule + "]. Illegal type.");
-      typeMap.put(lhs.charAt(0), rhs);
+      char c = lhs.charAt(0);
+      typeMap.put(c, rhs);
+      if (c > maxChar) {
+        maxChar = c;
+      }

Review comment:
       what is all this logic? This is a throw-away map (its just a local var and GC'd). Also it is bounded by Character so it can't have that many entries in it. Do we really need need to add this complexity?

##########
File path: lucene/core/src/java/org/apache/lucene/codecs/perfield/PerFieldPostingsFormat.java
##########
@@ -278,7 +278,8 @@ public void close() throws IOException {
 
   private static class FieldsReader extends FieldsProducer {
 
-    private final Map<String, FieldsProducer> fields = new TreeMap<>();
+    private final Map<String, FieldsProducer> fields = new HashMap<>();
+    private final List<String> fieldNames;

Review comment:
       TreeMap is a better choice than a map+list, that's too complicated.

##########
File path: lucene/core/src/java/org/apache/lucene/codecs/perfield/PerFieldDocValuesFormat.java
##########
@@ -262,9 +261,9 @@ static String getFullSegmentSuffix(String outerSegmentSuffix, String segmentSuff
     }
   }
 
-  private class FieldsReader extends DocValuesProducer {
+  private static class FieldsReader extends DocValuesProducer {
 
-    private final Map<String, DocValuesProducer> fields = new TreeMap<>();
+    private final Map<String, DocValuesProducer> fields = new HashMap<>();

Review comment:
       See also the other PR, with the default codec this is going to increase memory usage for people with tons of DV fields




-- 
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