You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by mayya-sharipova <gi...@git.apache.org> on 2018/02/03 01:45:32 UTC
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
GitHub user mayya-sharipova opened a pull request:
https://github.com/apache/lucene-solr/pull/320
LUCENE-8033: FieldInfos always use dense encoding
FieldInfos always to use an array to store FieldInfo byNumber
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mayya-sharipova/lucene-solr LUCENE-8033-FieldInfos
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/320.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #320
----
commit 04124e231fe7bb9d2d9f7964cd34d9d695fe641f
Author: Mayya Sharipova <ma...@...>
Date: 2018-02-03T01:10:24Z
LUCENE-8033: FieldInfos always use dense encoding
FieldInfos always use an array to store FieldInfo byNumber
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167482889
--- Diff: lucene/core/src/test/org/apache/lucene/index/TestFieldInfos.java ---
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+
+import java.util.Iterator;
+
+import org.apache.lucene.analysis.MockAnalyzer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.LuceneTestCase;
+
+public class TestFieldInfos extends LuceneTestCase {
+
+ public void testFieldInfos() throws Exception{
+ Directory dir = newDirectory();
+ IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig(new MockAnalyzer(random()))
+ .setMergePolicy(NoMergePolicy.INSTANCE));
+
+ Document d1 = new Document();
+ for (int i = 0; i < 15; i++) {
+ d1.add(new StringField("f" + i, "v" + i, Field.Store.YES));
+ }
+ writer.addDocument(d1);
+ writer.commit();
+
+ Document d2 = new Document();
+ d2.add(new StringField("f0", "v0", Field.Store.YES));
+ d2.add(new StringField("f15", "v15", Field.Store.YES));
+ d2.add(new StringField("f16", "v16", Field.Store.YES));
+ writer.addDocument(d2);
+ writer.commit();
+
+ Document d3 = new Document();
+ writer.addDocument(d3);
+ writer.close();
+
+ SegmentInfos sis = SegmentInfos.readLatestCommit(dir);
+ assertEquals(3, sis.size());
+
+ FieldInfos fis1 = IndexWriter.readFieldInfos(sis.info(0));
+ FieldInfos fis2 = IndexWriter.readFieldInfos(sis.info(1));
+ FieldInfos fis3 = IndexWriter.readFieldInfos(sis.info(2));
+
+
+ // testing dense FieldInfos
+ Iterator<FieldInfo> it = fis1.iterator();
+ int i = 0;
+ while(it.hasNext()) {
+ FieldInfo fi = it.next();
+ assertEquals(i, fi.number);
+ assertEquals("f" + i , fi.name);
+ i++;
+ }
+
+ // testing sparse FieldInfos
+ assertEquals("f0", fis2.fieldInfo(0).name);
+ assertNull(fis2.fieldInfo(1));
+ assertEquals("f15", fis2.fieldInfo(15).name);
+ assertEquals("f16", fis2.fieldInfo(16).name);
+
+ // testing empty FieldInfos
+ assertNull(fis3.fieldInfo(0));
+ assertEquals(0, fis3.size());
+ Iterator<FieldInfo> it3 = fis3.iterator();
+ assertFalse(it3.hasNext());
+ dir.close();
--- End diff --
in each of these 3 cases, can you also do some lookups by name and by number?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r166003726
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -96,25 +102,19 @@ public FieldInfos(FieldInfo[] infos) {
this.hasNorms = hasNorms;
this.hasDocValues = hasDocValues;
this.hasPointValues = hasPointValues;
- Integer max = byNumber.isEmpty() ? null : byNumber.lastKey();
-
- // Only usee TreeMap in the very sparse case (< 1/16th of the numbers are used),
- // because TreeMap uses ~ 64 (32 bit JVM) or 120 (64 bit JVM w/o compressed oops)
- // overall bytes per entry, but array uses 4 (32 bit JMV) or 8
- // (64 bit JVM w/o compressed oops):
- if (max != null && max < ArrayUtil.MAX_ARRAY_LENGTH && max < 16L*byNumber.size()) {
- // Pull infos into an arraylist to avoid holding a reference to the TreeMap
- values = Collections.unmodifiableCollection(new ArrayList<>(byNumber.values()));
- byNumberMap = null;
- byNumberTable = new FieldInfo[max+1];
- for (Map.Entry<Integer,FieldInfo> entry : byNumber.entrySet()) {
- byNumberTable[entry.getKey()] = entry.getValue();
+
+ List<FieldInfo> valuesTemp = new ArrayList<>();
+ if (size > 0){
+ byNumber = new FieldInfo[size];
+ for(int i=0; i<size; i++){
+ byNumber[i] = byNumberTemp[i];
+ if (byNumberTemp[i] != null)
+ valuesTemp.add(byNumberTemp[i]);
}
} else {
- byNumberMap = byNumber;
- values = Collections.unmodifiableCollection(byNumber.values());
- byNumberTable = null;
+ byNumber = null;
}
+ values = Collections.unmodifiableCollection(valuesTemp);
--- End diff --
Let's maybe allow empty slots at the end of the array list to be collected by doing something like this?
```java
values = Collections.unmodifiableCollection(Arrays.asList(valuesTemp.toArray(new FieldInfo[0])));
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r166006776
--- Diff: lucene/core/src/test/org/apache/lucene/index/TestFieldInfos.java ---
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+
+import java.util.Iterator;
+
+import org.apache.lucene.analysis.MockAnalyzer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.LuceneTestCase;
+import org.junit.Test;
+
+public class TestFieldInfos extends LuceneTestCase {
+
+ @Test
--- End diff --
We usually don't use this annotation and only rename on naming conventions: all methods that start with `test` are tested.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by mayya-sharipova <gi...@git.apache.org>.
Github user mayya-sharipova commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167649286
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
--- End diff --
@diegoceccarelli thanks for a suggestion. I considered `ArrayList` as well, but the code was even more complex. `byNumberTemp` could potentially be a sparse array, where some positions don't have values. You also need to `ensureCapacity` of the ArrayList, and `add` null elements.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #320: LUCENE-8033: FieldInfos always use dense encoding
Posted by mayya-sharipova <gi...@git.apache.org>.
Github user mayya-sharipova commented on the issue:
https://github.com/apache/lucene-solr/pull/320
@jpountz Adrien, thanks for your feedback. I tried to address it in the 2nd commit.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r166001771
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +60,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
for (FieldInfo info : infos) {
if (info.number < 0) {
throw new IllegalArgumentException("illegal field number: " + info.number + " for field " + info.name);
}
- FieldInfo previous = byNumber.put(info.number, info);
+ size = info.number >= size ? info.number+1 : size;
+ if (info.number >= capacity){ //grow array
+ capacity = info.number + 1;
+ byNumberTemp = Arrays.copyOf(byNumberTemp, capacity);
+ }
--- End diff --
can you grow the array exponentially so that it doesn't run in quadratic time in the worst case? ArrayUtil has some methods that make it easy:
```
if (info.number >= byNumberTemp.length) {
byNumberTemp = ArrayUtil.grow(byNumberTemp, info.number + 1);
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by mayya-sharipova <gi...@git.apache.org>.
Github user mayya-sharipova commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167653245
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
--- End diff --
@jpountz Thank you for the suggestion on `capacity` - very nice one. For the `size`, later we use this variable to create the final array: `byNumber = new FieldInfo[size]`. We can also do: `byNumber = new FieldInfo[byNumberTemp.length]`, but then `byNumber` may have some empty slots.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167482002
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
--- End diff --
as far as I'm concerned, I'm fine with either option: array or ArrayList.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167838894
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
--- End diff --
This looks more complex to me than manipulating the array directly?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucene-solr/pull/320
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by diegoceccarelli <gi...@git.apache.org>.
Github user diegoceccarelli commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167441676
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
--- End diff --
what not using `ArrayList` ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r166005204
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -96,25 +102,19 @@ public FieldInfos(FieldInfo[] infos) {
this.hasNorms = hasNorms;
this.hasDocValues = hasDocValues;
this.hasPointValues = hasPointValues;
- Integer max = byNumber.isEmpty() ? null : byNumber.lastKey();
-
- // Only usee TreeMap in the very sparse case (< 1/16th of the numbers are used),
- // because TreeMap uses ~ 64 (32 bit JVM) or 120 (64 bit JVM w/o compressed oops)
- // overall bytes per entry, but array uses 4 (32 bit JMV) or 8
- // (64 bit JVM w/o compressed oops):
- if (max != null && max < ArrayUtil.MAX_ARRAY_LENGTH && max < 16L*byNumber.size()) {
- // Pull infos into an arraylist to avoid holding a reference to the TreeMap
- values = Collections.unmodifiableCollection(new ArrayList<>(byNumber.values()));
- byNumberMap = null;
- byNumberTable = new FieldInfo[max+1];
- for (Map.Entry<Integer,FieldInfo> entry : byNumber.entrySet()) {
- byNumberTable[entry.getKey()] = entry.getValue();
+
+ List<FieldInfo> valuesTemp = new ArrayList<>();
+ if (size > 0){
+ byNumber = new FieldInfo[size];
+ for(int i=0; i<size; i++){
+ byNumber[i] = byNumberTemp[i];
+ if (byNumberTemp[i] != null)
+ valuesTemp.add(byNumberTemp[i]);
}
} else {
- byNumberMap = byNumber;
- values = Collections.unmodifiableCollection(byNumber.values());
- byNumberTable = null;
+ byNumber = null;
--- End diff --
It seems a bit trappy to use `null` to represent empty arrays, especially since the `fieldInfo` method doesn't test for nullity?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r166006885
--- Diff: lucene/core/src/test/org/apache/lucene/index/TestFieldInfos.java ---
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.index;
+
+
+import java.util.Iterator;
+
+import org.apache.lucene.analysis.MockAnalyzer;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.LuceneTestCase;
+import org.junit.Test;
+
+public class TestFieldInfos extends LuceneTestCase {
+
+ @Test
+ public void testFieldInfosSparse() throws Exception{
--- End diff --
Thanks for adding a test! Let's also add a similar test for empty and dense?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by jpountz <gi...@git.apache.org>.
Github user jpountz commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167482757
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
--- End diff --
I think we can simplify a bit by removing these two local variables:
- `capacity` can be replaced with byNumberTemp.length
- `size` seems only useful to terminate early when creating a list of the field infos from byNumberTemp. But since ArrayUtil.grow is used, arrays are never more than ~1/8th larger than the number of slots that are actually used, so it doesn't buy much?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #320: LUCENE-8033: FieldInfos always use dense enco...
Posted by diegoceccarelli <gi...@git.apache.org>.
Github user diegoceccarelli commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/320#discussion_r167671548
--- Diff: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java ---
@@ -63,21 +62,30 @@ public FieldInfos(FieldInfo[] infos) {
boolean hasNorms = false;
boolean hasDocValues = false;
boolean hasPointValues = false;
-
- TreeMap<Integer, FieldInfo> byNumber = new TreeMap<>();
+
+ int size = 0; // number of elements in byNumberTemp
+ int capacity = 10; // byNumberTemp's capacity
+ FieldInfo[] byNumberTemp = new FieldInfo[capacity];
--- End diff --
That's true :) what about having an `ArrayList` and then a separate private method `ensureSize` ?
```
private static void ensureSize(ArrayList<?> list, int size) {
list.ensureCapacity(size);
while (list.size() < size) {
list.add(null);
}
}
```
In this way you will not have to care about the logic behind the resizing and you will have less variables around.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org