You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "iverase (via GitHub)" <gi...@apache.org> on 2024/10/23 14:21:52 UTC
[PR] Allow reading binary doc values as a RandomAccessInput [lucene]
iverase opened a new pull request, #13948:
URL: https://github.com/apache/lucene/pull/13948
Following up this [suggestion](https://github.com/apache/lucene/pull/12460#issuecomment-1715126194) from @jpountz, here I propose to add a new method to the BinaryDocValues API that returns the content of the doc values as a RandomAccessInput. This will allow to have an off-heap version of the binary doc value whenever possible.
The main two changes here are:
1) Lucene90DocValuesProducer allocates the BytesRef for binary doc values lazily.
2) To make transparent when the binary doc value is already on heap, BytesRef implements RandomAccessInput so we can just return the result from binaryValue() in those cases. If that's not ok, we can create a RandomAccessInput wrapper for BytesRef instead.
relates https://github.com/apache/lucene/issues/12459
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1877891198
##########
lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextDocValuesReader.java:
##########
@@ -383,17 +392,31 @@ public long cost() {
@Override
public int advance(int target) throws IOException {
- return docsWithField.advance(target);
+ int doc = docsWithField.advance(target);
+ if (doc != NO_MORE_DOCS) {
+ consume(values.apply(doc));
+ }
+ return doc;
+ }
+
+ private void consume(BytesRef bytesRef) {
+ bytes.reset(bytesRef.bytes);
+ inputRef.offset = bytesRef.offset;
+ inputRef.length = bytesRef.length;
}
@Override
public boolean advanceExact(int target) throws IOException {
- return docsWithField.advanceExact(target);
+ if (docsWithField.advanceExact(target)) {
+ consume(values.apply(target));
+ return true;
+ }
+ return false;
}
@Override
- public BytesRef binaryValue() throws IOException {
- return values.apply(docsWithField.docID());
+ public RandomAccessInputRef randomAccessInputValue() {
+ return inputRef;
Review Comment:
None, I rework it so we call `values#apply` only in `randomAccessInputValue()`
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1812935475
##########
lucene/core/src/java/org/apache/lucene/index/BinaryDocValues.java:
##########
@@ -33,4 +34,15 @@ protected BinaryDocValues() {}
* @return binary value
*/
public abstract BytesRef binaryValue() throws IOException;
+
+ /**
+ * Returns the binary value as a {@link RandomAccessInput} for the current document ID. The bytes
+ * start at position 0 up to {@link RandomAccessInput#length()}. It is illegal to call this method
+ * after {@link #advanceExact(int)} returned {@code false}.
Review Comment:
Can you also clarify that the same instance may be reused across calls, so callers should make sure to fully consume their RandomAccessInput instance before moving to the next doc?
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+
+ private long pos;
+
+ public RandomAccessInputDataInput() {}
+
+ // NOTE: sets pos to 0, which is not right if you had
+ // called reset w/ non-zero offset!!
+ public void rewind() {
+ pos = 0;
+ }
+
+ public long getPosition() {
+ return pos;
+ }
+
+ public void setPosition(long pos) {
+ this.pos = pos;
+ }
+
+ public void reset(RandomAccessInput input) {
+ this.input = input;
+ pos = 0;
+ }
Review Comment:
Can you add javadocs to the above methods?
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+
+ private long pos;
+
+ public RandomAccessInputDataInput() {}
+
+ // NOTE: sets pos to 0, which is not right if you had
+ // called reset w/ non-zero offset!!
+ public void rewind() {
+ pos = 0;
+ }
+
+ public long getPosition() {
+ return pos;
+ }
+
+ public void setPosition(long pos) {
+ this.pos = pos;
+ }
+
+ public void reset(RandomAccessInput input) {
+ this.input = input;
+ pos = 0;
+ }
+
+ public long length() {
+ return input.length();
+ }
+
+ @Override
+ public void skipBytes(long count) {
+ pos += count;
+ }
+
+ @Override
+ public short readShort() throws IOException {
+ try {
+ return input.readShort(pos);
+ } finally {
+ pos += Short.BYTES;
Review Comment:
This finally block feels wrong: the position should not be updated if the call raises an 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.
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2439897160
I have a second iteration and I realise that this change can be easily done if we introduce a new abstraction. I have called it `RandomAccessInputRef` and it is equivalent to BytesRef but backed by a RandomAccessInput instead of a byte array. I am thinking of it s the off-heap version of BytesRef.
Using this, it makes the changes almost mechanical. The big difference between BytesRef and RandomAccessInputRef is that the latter cannot implement efficient the Compare interface so it cannot be use for comparisons and equality. In that case we need to read bytes on-heap.
With this new abstraction, we just need to add three new methods:
- BytesRefBuilder#copyBytes(RandomAccessInputRef in) throws IOException
- BytesRefArray#append(RandomAccessInputRef bytes) throws IOException
- ByteBlockPool#append(final RandomAccessInput bytes, long offset, int length) throws IOException: for supporting the method above)
-
And two helper classes:
- ByteArrayRandomAccessInput: To represent and byte array as a RandomAccessInput
- RandomAccessInputDataInput: to wrap a RandomAccessInput as a DataInput so we can use DataOutput#copyBytes(DataInput input, long numBytes)
There are few places that still needs to be adapted to take full advantage of the new API but they can be done in a follow up as they require a bit of work :
- MatchingFacetSetsCounts
- BaseShapeDocValuesQuery
- SerializedDVStrategy
- MonitorQuerySerializer
And there are two places that we will need to put the data on-heap as they are using equality:
- TermValComparator
- BinaryDocValueSelector
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2709158794
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2568551704
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1877904767
##########
lucene/core/src/java/org/apache/lucene/search/FieldComparator.java:
##########
@@ -234,7 +235,8 @@ public TermValComparator(int numHits, String field, boolean sortMissingLast) {
private BytesRef getValueForDoc(int doc) throws IOException {
if (docTerms.advanceExact(doc)) {
- return docTerms.binaryValue();
+ bytesRefBuilder.copyBytes(docTerms.randomAccessInputValue());
+ return bytesRefBuilder.get();
Review Comment:
agree, it will require a bit of rework here but should be doable. Maybe as a follow up?
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1816323891
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+
+ private long pos;
+
+ public RandomAccessInputDataInput() {}
+
+ // NOTE: sets pos to 0, which is not right if you had
+ // called reset w/ non-zero offset!!
+ public void rewind() {
+ pos = 0;
+ }
+
+ public long getPosition() {
+ return pos;
+ }
+
+ public void setPosition(long pos) {
+ this.pos = pos;
+ }
+
+ public void reset(RandomAccessInput input) {
+ this.input = input;
+ pos = 0;
+ }
+
+ public long length() {
+ return input.length();
+ }
+
+ @Override
+ public void skipBytes(long count) {
+ pos += count;
+ }
+
+ @Override
+ public short readShort() throws IOException {
+ try {
+ return input.readShort(pos);
+ } finally {
+ pos += Short.BYTES;
Review Comment:
This class is a copy / paste from ByteArrayDataInput so it makes me wonder if that's something we need to change in that implementation too.
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2902069576
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the 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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1816322441
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+
+ private long pos;
+
+ public RandomAccessInputDataInput() {}
+
+ // NOTE: sets pos to 0, which is not right if you had
+ // called reset w/ non-zero offset!!
+ public void rewind() {
+ pos = 0;
+ }
+
+ public long getPosition() {
+ return pos;
+ }
+
+ public void setPosition(long pos) {
+ this.pos = pos;
+ }
+
+ public void reset(RandomAccessInput input) {
+ this.input = input;
+ pos = 0;
+ }
Review Comment:
done
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1816321990
##########
lucene/core/src/java/org/apache/lucene/index/BinaryDocValues.java:
##########
@@ -33,4 +34,15 @@ protected BinaryDocValues() {}
* @return binary value
*/
public abstract BytesRef binaryValue() throws IOException;
+
+ /**
+ * Returns the binary value as a {@link RandomAccessInput} for the current document ID. The bytes
+ * start at position 0 up to {@link RandomAccessInput#length()}. It is illegal to call this method
+ * after {@link #advanceExact(int)} returned {@code false}.
Review Comment:
done
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2673020214
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1877892464
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+import org.apache.lucene.util.RandomAccessInputRef;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+ private long offset;
+ private long length;
+
+ private long pos;
Review Comment:
Done, I rework it so it looks like `ByteArrayDataInput`
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1877894071
##########
lucene/core/src/java/org/apache/lucene/util/UnicodeUtil.java:
##########
@@ -627,35 +629,58 @@ public static String toHexString(String s) {
}
/**
- * Interprets the given byte array as UTF-8 and converts to UTF-16. It is the responsibility of
- * the caller to make sure that the destination array is large enough.
+ * Interprets the given {@link RandomAccessInput} slice as UTF-8 and converts to UTF-16. It is the
+ * responsibility of the caller to make sure that the destination array is large enough.
+ *
+ * <p>NOTE: Full characters are read, even if this reads past the length passed (and can result in
+ * an IOException if invalid UTF-8 is passed). Explicit checks for valid UTF-8 are not performed.
+ */
+ // TODO: broken if chars.offset != 0
+ public static int UTF8toUTF16(RandomAccessInput input, long offset, int length, char[] out)
Review Comment:
I revert this changes. If we see it is performance sensitive we can re add it.
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2951296677
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1876152549
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90DocValuesProducer.java:
##########
@@ -791,25 +792,26 @@ public BinaryDocValues getBinary(FieldInfo field) throws IOException {
return DocValues.emptyBinary();
}
- final RandomAccessInput bytesSlice = data.randomAccessSlice(entry.dataOffset, entry.dataLength);
+ final RandomAccessInputRef bytesSlice =
+ new RandomAccessInputRef(data.randomAccessSlice(entry.dataOffset, entry.dataLength));
Review Comment:
Nit, maybe extract two variables: `bytesSlice` and `byteSliceRef` to make it a bit easier to read?
##########
lucene/codecs/src/java/org/apache/lucene/codecs/simpletext/SimpleTextDocValuesReader.java:
##########
@@ -383,17 +392,31 @@ public long cost() {
@Override
public int advance(int target) throws IOException {
- return docsWithField.advance(target);
+ int doc = docsWithField.advance(target);
+ if (doc != NO_MORE_DOCS) {
+ consume(values.apply(doc));
+ }
+ return doc;
+ }
+
+ private void consume(BytesRef bytesRef) {
+ bytes.reset(bytesRef.bytes);
+ inputRef.offset = bytesRef.offset;
+ inputRef.length = bytesRef.length;
}
@Override
public boolean advanceExact(int target) throws IOException {
- return docsWithField.advanceExact(target);
+ if (docsWithField.advanceExact(target)) {
+ consume(values.apply(target));
+ return true;
+ }
+ return false;
}
@Override
- public BytesRef binaryValue() throws IOException {
- return values.apply(docsWithField.docID());
+ public RandomAccessInputRef randomAccessInputValue() {
+ return inputRef;
Review Comment:
What was the problem with calling `values#apply` in `randomAccessInputValue()` rather than in `nextDoc()`/`advance()`/`advanceExact()`?
##########
lucene/core/src/java/org/apache/lucene/util/UnicodeUtil.java:
##########
@@ -627,35 +629,58 @@ public static String toHexString(String s) {
}
/**
- * Interprets the given byte array as UTF-8 and converts to UTF-16. It is the responsibility of
- * the caller to make sure that the destination array is large enough.
+ * Interprets the given {@link RandomAccessInput} slice as UTF-8 and converts to UTF-16. It is the
+ * responsibility of the caller to make sure that the destination array is large enough.
+ *
+ * <p>NOTE: Full characters are read, even if this reads past the length passed (and can result in
+ * an IOException if invalid UTF-8 is passed). Explicit checks for valid UTF-8 are not performed.
+ */
+ // TODO: broken if chars.offset != 0
+ public static int UTF8toUTF16(RandomAccessInput input, long offset, int length, char[] out)
Review Comment:
Having `UnicodeUtil` need to work on a `RandomAccessInput` doesn't feel right. Do you know if this is performance-sensitive? Otherwise I'd rather make `RandomAccessInputRef` implement `utf8ToString()` by converting to a `BytesRef` first.
##########
lucene/core/src/java/org/apache/lucene/store/RandomAccessInputDataInput.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.store;
+
+import java.io.IOException;
+import org.apache.lucene.util.RandomAccessInputRef;
+
+/**
+ * DataInput backed by a {@link RandomAccessInput}. <b>WARNING:</b> This class omits all low-level
+ * checks.
+ *
+ * @lucene.experimental
+ */
+public final class RandomAccessInputDataInput extends DataInput {
+
+ private RandomAccessInput input;
+ private long offset;
+ private long length;
+
+ private long pos;
Review Comment:
I wonderif `pos` should start at `offset` rather than `0` so that we don't have to sum up `offset` and `pos` whenever we need to know the current position?
##########
lucene/core/src/java/org/apache/lucene/index/BinaryDocValues.java:
##########
@@ -18,19 +18,20 @@
package org.apache.lucene.index;
import java.io.IOException;
-import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.RandomAccessInputRef;
-/** A per-document numeric value. */
+/** A per-document binary value. */
Review Comment:
Woops, a 10 years old typo. :)
##########
lucene/core/src/java/org/apache/lucene/search/FieldComparator.java:
##########
@@ -234,7 +235,8 @@ public TermValComparator(int numHits, String field, boolean sortMissingLast) {
private BytesRef getValueForDoc(int doc) throws IOException {
if (docTerms.advanceExact(doc)) {
- return docTerms.binaryValue();
+ bytesRefBuilder.copyBytes(docTerms.randomAccessInputValue());
+ return bytesRefBuilder.get();
Review Comment:
Nit: it's a bit sad to load all bytes on top heap, when in many cases we could identify that the value is not competitive after reading a few bytes 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.
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1888624444
##########
lucene/core/src/test/org/apache/lucene/util/TestBytesRefArray.java:
##########
@@ -43,8 +44,17 @@ public void testAppend() throws IOException {
for (int i = 0; i < entries; i++) {
String randomRealisticUnicodeString = TestUtil.randomRealisticUnicodeString(random);
spare.copyChars(randomRealisticUnicodeString);
- assertEquals(i + initSize, list.append(spare.get()));
+ if (random().nextBoolean()) {
+ BytesRef bytesRef = spare.get();
+ RandomAccessInputRef inputRef =
+ new RandomAccessInputRef(
+ new ByteArrayRandomAccessInput(bytesRef.bytes), bytesRef.offset, bytesRef.length);
+ assertEquals(i + initSize, list.append(inputRef));
+ } else {
+ assertEquals(i + initSize, list.append(spare.get()));
+ }
stringList.add(randomRealisticUnicodeString);
+ ;
Review Comment:
remove?
##########
lucene/monitor/src/java/org/apache/lucene/monitor/MonitorQuerySerializer.java:
##########
@@ -48,38 +47,31 @@ public interface MonitorQuerySerializer {
*/
static MonitorQuerySerializer fromParser(Function<String, Query> parser) {
return new MonitorQuerySerializer() {
+
@Override
- public MonitorQuery deserialize(BytesRef binaryValue) {
- ByteArrayInputStream is =
- new ByteArrayInputStream(binaryValue.bytes, binaryValue.offset, binaryValue.length);
- try (InputStreamDataInput data = new InputStreamDataInput(is)) {
- String id = data.readString();
- String query = data.readString();
- Map<String, String> metadata = new HashMap<>();
- for (int i = data.readInt(); i > 0; i--) {
- metadata.put(data.readString(), data.readString());
- }
- return new MonitorQuery(id, parser.apply(query), query, metadata);
- } catch (IOException e) {
- throw new RuntimeException(e); // shouldn't happen, we're reading from a bytearray!
+ public MonitorQuery deserialize(RandomAccessInputRef input) throws IOException {
+ RandomAccessInputDataInput data = new RandomAccessInputDataInput();
+ data.reset(input);
+ String id = data.readString();
+ String query = data.readString();
+ Map<String, String> metadata = new HashMap<>();
+ for (int i = data.readInt(); i > 0; i--) {
+ metadata.put(data.readString(), data.readString());
}
+ return new MonitorQuery(id, parser.apply(query), query, metadata);
Review Comment:
Nice that it doesn't load everything it a giant byte[] anymore!
##########
lucene/core/src/java/org/apache/lucene/util/RandomAccessInputRef.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import org.apache.lucene.store.RandomAccessInput;
+
+/**
+ * Represents a RandomAccessInput, as a slice (offset + length) into an existing RandomAccessInput.
+ * The {@link #bytes} member should never be null. In many ways, it is the off-heap equivalent of a
+ * {@link BytesRef}.
+ *
+ * @see BytesRef
+ */
+public final class RandomAccessInputRef {
+
+ /** The contents of the RandomAccessInput. */
+ public RandomAccessInput bytes;
+
+ /** Offset of first valid byte. */
+ public long offset;
+
+ /** Length of used bytes. */
+ public int length;
+
+ public RandomAccessInputRef(RandomAccessInput bytes) {
+ this(bytes, 0, 0);
+ }
+
+ public RandomAccessInputRef(RandomAccessInput bytes, long offset, int length) {
+ this.bytes = bytes;
+ this.offset = offset;
+ this.length = length;
+ }
+
+ /**
+ * Interprets stored bytes as UTF-8 bytes, returning the resulting string. May throw an {@link
+ * AssertionError} or a {@link RuntimeException} if the data is not well-formed UTF-8.
+ */
+ public String utf8ToString() throws IOException {
+ final char[] ref = new char[length];
+ final int len = UnicodeUtil.UTF8toUTF16(toBytesRef(this), ref);
+ return new String(ref, 0, len);
+ }
+
+ /**
+ * Creates a new BytesRef that points to a copy of the bytes from <code>input</code> starting at
+ * offset for length.
+ *
+ * <p>The returned BytesRef will have a offset of zero.
+ */
+ public static BytesRef toBytesRef(RandomAccessInputRef input) throws IOException {
Review Comment:
I'd be keen to make it a method rather than a static function?
##########
lucene/core/src/test/org/apache/lucene/index/Test2BBinaryDocValues.java:
##########
@@ -88,12 +89,13 @@ public void testFixedBinary() throws Exception {
LeafReader reader = context.reader();
BinaryDocValues dv = reader.getBinaryDocValues("dv");
for (int i = 0; i < reader.maxDoc(); i++) {
+ ;
Review Comment:
remove?
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-3408723289
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2437732473
In my experience, binary doc values are more often used to encode structured data, such as maps that help build scoring signals, geo shapes, etc. than actual binary content, so this change makes sense to me. I'm interested in having more opinions though.
Would be nice to extend AssertingBinaryDocValues to make sure that all reads in the input are within bounds.
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13948:
URL: https://github.com/apache/lucene/pull/13948#issuecomment-2467011723
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Allow reading binary doc values as a RandomAccessInput [lucene]
Posted by "iverase (via GitHub)" <gi...@apache.org>.
iverase commented on code in PR #13948:
URL: https://github.com/apache/lucene/pull/13948#discussion_r1890139004
##########
lucene/core/src/java/org/apache/lucene/util/RandomAccessInputRef.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.util;
+
+import java.io.IOException;
+import org.apache.lucene.store.RandomAccessInput;
+
+/**
+ * Represents a RandomAccessInput, as a slice (offset + length) into an existing RandomAccessInput.
+ * The {@link #bytes} member should never be null. In many ways, it is the off-heap equivalent of a
+ * {@link BytesRef}.
+ *
+ * @see BytesRef
+ */
+public final class RandomAccessInputRef {
+
+ /** The contents of the RandomAccessInput. */
+ public RandomAccessInput bytes;
+
+ /** Offset of first valid byte. */
+ public long offset;
+
+ /** Length of used bytes. */
+ public int length;
+
+ public RandomAccessInputRef(RandomAccessInput bytes) {
+ this(bytes, 0, 0);
+ }
+
+ public RandomAccessInputRef(RandomAccessInput bytes, long offset, int length) {
+ this.bytes = bytes;
+ this.offset = offset;
+ this.length = length;
+ }
+
+ /**
+ * Interprets stored bytes as UTF-8 bytes, returning the resulting string. May throw an {@link
+ * AssertionError} or a {@link RuntimeException} if the data is not well-formed UTF-8.
+ */
+ public String utf8ToString() throws IOException {
+ final char[] ref = new char[length];
+ final int len = UnicodeUtil.UTF8toUTF16(toBytesRef(this), ref);
+ return new String(ref, 0, len);
+ }
+
+ /**
+ * Creates a new BytesRef that points to a copy of the bytes from <code>input</code> starting at
+ * offset for length.
+ *
+ * <p>The returned BytesRef will have a offset of zero.
+ */
+ public static BytesRef toBytesRef(RandomAccessInputRef input) throws IOException {
Review Comment:
I think I make it static to mimic the DeepCopy method on BytesRef but it loos nicer as a method.
--
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