You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "amit-jain (via GitHub)" <gi...@apache.org> on 2023/08/16 05:30:56 UTC
[GitHub] [jackrabbit-oak] amit-jain opened a new pull request, #1071: OAK-10384: Fix stripping of large indexed ordered properties
amit-jain opened a new pull request, #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071
- Convert to BytesRef and then recheck and trim by the amount exceeding
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319589400
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
Review Comment:
Ok makes sense to add a clarifying comment that lucene limits the length and the length that is checked is the BytesRef.length
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295553491
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ String truncated = value;
+ if (value.length() > maxLength) {
+ log.warn("Truncating property {} having length {} at path:[{}] as it is > {}", prop, value.length(), path, maxLength);
+ truncated = value.substring(0, maxLength);
Review Comment:
Ah I understood now yes it's a problem for surrogate pairs
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318358674
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
I don't think that splitting will make it clearer though. The method has the check interwined also to check the higher code points are not split.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318360063
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
Review Comment:
Could we add examples of these skipping and removals of multi-byte sequences? And also add explanations for why they are necessary?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318377448
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
I think the function does quite a lot of things - and I generally like our code to follow the SRP. Some minor refactoring can lead to this, which IMO is a bit clearer:
```java
// TODO: explain why we use 10
private static final int EMERGENCY_TRUNCATION_LENGTH = 10;
...
// assuming we rename checkTruncateLength() -> truncateToMaxLength()
protected static BytesRef truncateToMaxLength(String prop, String value, String path, int maxLength) {
log.trace("Property {} at path:[{}] has value {}", prop, path, value);
BytesRef ref = new BytesRef(value);
if (ref.length <= maxLength) {
return ref;
}
ref = truncateBytesRef(ref, maxLength);
if (ref.length > maxLength) {
ref = emergencyTruncate(ref, maxLength);
}
return ref;
}
private static BytesRef truncateBytesRef(BytesRef ref, int maxLength) {
log.info("Truncating as length after encoding {} is > {} ", ref.length, maxLength);
int end = calculateEndPosition(ref, maxLength);
byte[] truncatedBytes = Arrays.copyOf(ref.bytes, end + 1);
BytesRef truncatedRef = new BytesRef(new String(truncatedBytes, StandardCharsets.UTF_8));
log.trace("Truncated to {}", truncatedRef.utf8ToString());
return truncatedRef;
}
// TODO: explain this more in detail, with examples and why it's necessary.
private static int calculateEndPosition(BytesRef ref, int maxLength) {
int end = maxLength - 1;
while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
end--;
}
if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
end--;
}
return end;
}
// TODO: explain why this is necessary and with examples.
private static BytesRef emergencyTruncate(BytesRef ref, int maxLength) {
log.error("Truncation did not work: still {} bytes", ref.length);
String truncated = ref.utf8ToString();
while (ref.length > maxLength) {
truncated = truncated.substring(0, truncated.length() - EMERGENCY_TRUNCATION_LENGTH);
ref = new BytesRef(truncated);
}
return ref;
}
```
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318352601
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
+ end--;
+ }
+ // remove one head of a utf-8 multi-byte sequence (at most 1)
+ if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
+ end--;
+ }
+ byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
+ String truncated = new String(bytes2, StandardCharsets.UTF_8);
+ ref = new BytesRef(truncated);
+ log.trace("Truncated property {} at path:[{}] to {}", prop, path, ref.utf8ToString());
+ while (ref.length > maxLength) {
Review Comment:
I think it's important to document this "emergency" even more. When can it happen, what do we do to fix it, and why is it okay for us to fix it the way we are? From a quick glance at the code, it is not immediately clear why this works.
Specifically, one or more clear examples would make it very clear imo.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318389192
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
+ end--;
+ }
+ // remove one head of a utf-8 multi-byte sequence (at most 1)
+ if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
+ end--;
+ }
+ byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
Review Comment:
Yes that makes sense.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319554989
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
Review Comment:
Thomas linked this one a few months ago that I found very useful: https://writers.com/concise-writing
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318385793
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
+ end--;
+ }
+ // remove one head of a utf-8 multi-byte sequence (at most 1)
+ if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
+ end--;
+ }
+ byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
+ String truncated = new String(bytes2, StandardCharsets.UTF_8);
+ ref = new BytesRef(truncated);
+ log.trace("Truncated property {} at path:[{}] to {}", prop, path, ref.utf8ToString());
+ while (ref.length > maxLength) {
Review Comment:
As the original comment noted it should never happen but Thomas added for unforseen cases rather than just throeing an error.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295535438
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ String truncated = value;
+ if (value.length() > maxLength) {
Review Comment:
I think this could lead to a null pointer exception. From what I can see, we aren't guaranteed that it is not null.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319566342
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
+ * of the {@code BytesRef} object to the specified {@code maxLength}, ensuring that the multi-byte sequences are
+ * properly truncated.
+ *
+ * <p>The {@code BytesRef} object is created from the provided {@code String} value using UTF-8 encoding. As a result, its length
+ * can exceed that of the {@code String} value, since Java strings use UTF-16 encoding. This necessitates appropriate truncation.
+ *
+ * <p>Multi-byte sequences will be of the form {@code 11xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx}.
+ * The method first truncates continuation bytes, which start with {@code 10} in binary. It then truncates the head byte, which
+ * starts with {@code 11}. Both truncation operations use a binary mask of {@code 11100000}.
Review Comment:
Do both truncation operations really use a binary mask of `11100000`?
From what I can see, the first while loop:
```java
while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
end--;
}
```
is checking if the byte at `ref.bytes[end]` is of the form `10xxxxxx`. It's doing that by masking the byte with `11000000` and checking if the result is `10000000`. If it is, then the byte has the form `10xxxxxx`.
And the following `if` statement:
```java
if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
end--;
}
```
checks in a similar manner if the byte-sequence is of the pattern `110xxxxx`. But I don't see that any of the truncation operations use a binary mask of `11100000`.
In any case, I think this operation is complex enough that I'd be in strong favour of refactoring it into its own function and documented accordingly. That way, writing a proper example wouldn't clutter the `getTruncatedBytesRef` function.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295506792
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ String truncated = value;
+ if (value.length() > maxLength) {
+ log.warn("Truncating property {} having length {} at path:[{}] as it is > {}", prop, value.length(), path, maxLength);
+ truncated = value.substring(0, maxLength);
Review Comment:
This will not truncate correctly if there are surrogate pairs
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318377448
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
I think the function does quite a lot of things - and I generally like our code to follow the SRP. Some minor refactoring can lead to this, which IMO is a bit clearer. It is more code, but I think it's clearer - especially when we fix the TODOs for new people to read it.
```java
// TODO: explain why we use 10
private static final int EMERGENCY_TRUNCATION_LENGTH = 10;
...
// assuming we rename checkTruncateLength() -> truncateToMaxLength()
protected static BytesRef truncateToMaxLength(String prop, String value, String path, int maxLength) {
log.trace("Property {} at path:[{}] has value {}", prop, path, value);
BytesRef ref = new BytesRef(value);
if (ref.length <= maxLength) {
return ref;
}
ref = truncateBytesRef(ref, maxLength);
if (ref.length > maxLength) {
ref = emergencyTruncate(ref, maxLength);
}
return ref;
}
private static BytesRef truncateBytesRef(BytesRef ref, int maxLength) {
log.info("Truncating as length after encoding {} is > {} ", ref.length, maxLength);
int end = calculateEndPosition(ref, maxLength);
byte[] truncatedBytes = Arrays.copyOf(ref.bytes, end + 1);
BytesRef truncatedRef = new BytesRef(new String(truncatedBytes, StandardCharsets.UTF_8));
log.trace("Truncated to {}", truncatedRef.utf8ToString());
return truncatedRef;
}
// TODO: explain this more in detail, with examples and why it's necessary.
private static int calculateEndPosition(BytesRef ref, int maxLength) {
int end = maxLength - 1;
while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
end--;
}
if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
end--;
}
return end;
}
// TODO: explain why this is necessary and with examples.
private static BytesRef emergencyTruncate(BytesRef ref, int maxLength) {
log.error("Truncation did not work: still {} bytes", ref.length);
String truncated = ref.utf8ToString();
while (ref.length > maxLength) {
truncated = truncated.substring(0, truncated.length() - EMERGENCY_TRUNCATION_LENGTH);
ref = new BytesRef(truncated);
}
return ref;
}
```
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318388488
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
Hmm...I think this takes the refactoring too far and in fact makes readability harder. Generally if there was a need for reuse for some of the sub methods it would have made sense. Explanation of the code can happen through documentation as well.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319554989
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
Review Comment:
Thomas linked this one a few months ago that I found very useful: https://writers.com/concise-writing
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain merged pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain merged PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318718146
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
But if you don't like the refactoring that's okay. I think what's most crucial is that we just document why we are doing those magical things - with some examples as well.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318352601
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
+ end--;
+ }
+ // remove one head of a utf-8 multi-byte sequence (at most 1)
+ if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
+ end--;
+ }
+ byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
+ String truncated = new String(bytes2, StandardCharsets.UTF_8);
+ ref = new BytesRef(truncated);
+ log.trace("Truncated property {} at path:[{}] to {}", prop, path, ref.utf8ToString());
+ while (ref.length > maxLength) {
Review Comment:
I think it's important to document this "emergency" even more. When can it happen, what do we do to fix it, and why is it okay for us to fix it the way we are. From a quick glance at the code, it is not immediately clear why this works.
Specifically, one or more clear examples would make it very clear imo.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318274102
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
Could we change the name of the function to `truncateToMaxLength`, `getTruncatedBytesRef` something like that? IMO `checkTruncateLength` suggests that the function might just be verifying something rather than actually performing a truncation operation.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318318315
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
`checkTruncateLength` is meant to be `checkAndTruncate` and I removd And for brevity. So, maybe `checkAndTruncateLength` if its more clearer
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "fabriziofortino (via GitHub)" <gi...@apache.org>.
fabriziofortino commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318385118
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
+ end--;
+ }
+ // remove one head of a utf-8 multi-byte sequence (at most 1)
+ if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
+ end--;
+ }
+ byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
Review Comment:
for clarity, I would use something like `truncatedBytes` instead of `bytes2`
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#issuecomment-1680121413
I would make the truncation method public and then write a good unit test for it.
private static final Logger log = LoggerFactory.getLogger(LuceneDocumentMaker.class);
@Test
public void test() {
Random r = new Random(1);
for (int i = 0; i < 1000; i++) {
String x = randomUnicodeString(r, 5);
BytesRef ref = checkTruncateLength("x", x, "/x", 5);
assertTrue(ref.length > 0 && ref.length <= 5);
}
}
private String randomUnicodeString(Random r, int len) {
StringBuilder buff = new StringBuilder();
for(int i=0; i<len; i++) {
// see https://en.wikipedia.org/wiki/UTF-8
switch (r.nextInt(6)) {
case 2:
// 2 UTF-8 bytes
buff.append('£');
break;
case 3:
// 3 UTF-8 bytes
buff.append('€');
break;
case 4:
// 4 UTF-8 bytes
buff.append("\uD800\uDF48");
break;
default:
// most cases:
// 1 UTF-8 byte (ASCII)
buff.append('$');
}
}
return buff.toString();
}
public static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
BytesRef ref = new BytesRef(value);
if (ref.length <= maxLength) {
return ref;
}
log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
prop, path, ref.length, maxLength);
int end = maxLength - 1;
// skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
end--;
}
// remove one head of a utf-8 multi-byte sequence (at most 1)
if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
end--;
}
byte[] bytes2 = Arrays.copyOf(ref.bytes, end + 1);
String truncated = new String(bytes2, StandardCharsets.UTF_8);
ref = new BytesRef(truncated);
while (ref.length > maxLength) {
log.error("Truncation did not work: still {} bytes", ref.length);
// this may not properly work with unicode surrogates:
// it is an "emergency" procedure and should never happen
truncated = truncated.substring(0, truncated.length() - 10);
ref = new BytesRef(truncated);
}
return ref;
}
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295461538
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
This is not always working properly. E.g. this throws a StringIndexOutOfBoundsException:
checkTruncateLength("x", "$$$$€£$$", "/x", 10)
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
This is not always working properly. E.g. this throws a StringIndexOutOfBoundsException:
checkTruncateLength("x", "$$$$€£$$", "/x", 10)
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318633557
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
You know, I see where you're coming from about the code length. But I believe that while it might appear longer, it doesn't reduce readability. In fact, breaking the code down helps us describe and understand what each segment is doing, and more crucially, makes it easier to explain _why_ it's doing that.
Imagine we come across a pesky bug half a year from now and then stumble upon this piece of code:
```java
while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
end--;
}
if ((ref.bytes[end] & 0b11000000) == 0b11000000) {
end--;
}
```
Yeah I see that it skips over tails of utf-8 multi-byte sequences up to 3 bytes. But why?
Thus, wouldn't it be so much easier if we immediately knew the reason behind this logic, instead of scratching our heads trying to decode it? That's why I believe refactoring it is so crucial - it makes it so much easier to explain _why_ we are doing 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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318384289
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ log.trace("Property {} at path:[{}] has value {}", prop, path, value);
+
+ BytesRef ref = new BytesRef(value);
+ if (ref.length <= maxLength) {
+ return ref;
+ }
+ log.info("Truncating property {} at path:[{}] as length after encoding {} is > {} ",
+ prop, path, ref.length, maxLength);
+ int end = maxLength - 1;
+ // skip over tails of utf-8 multi-byte sequences (up to 3 bytes)
+ while ((ref.bytes[end] & 0b11000000) == 0b10000000) {
Review Comment:
There are tests available in LuceneLargeStringPropertyTest.java. Is tht not enough, maybe I can add a bit of documentation.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295518756
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ String truncated = value;
+ if (value.length() > maxLength) {
+ log.warn("Truncating property {} having length {} at path:[{}] as it is > {}", prop, value.length(), path, maxLength);
+ truncated = value.substring(0, maxLength);
Review Comment:
The truncation is on the original string so, it should be fine.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1295535438
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +312,29 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ private static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
+ String truncated = value;
+ if (value.length() > maxLength) {
Review Comment:
I think this could lead to a null pointer 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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318318315
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
`checkTruncateLength` is meant to be `checkAndTruncate` and I removd `And` for brevity. So, maybe `checkAndTruncateLength` if its more clearer
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1318335484
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -315,6 +313,38 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
}
return fieldAdded;
}
+
+ protected static BytesRef checkTruncateLength(String prop, String value, String path, int maxLength) {
Review Comment:
Perhaps it would then be even clearer to split the function in two: one that truncates and the other checks?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] amit-jain commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "amit-jain (via GitHub)" <gi...@apache.org>.
amit-jain commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319588274
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
+ * of the {@code BytesRef} object to the specified {@code maxLength}, ensuring that the multi-byte sequences are
+ * properly truncated.
+ *
+ * <p>The {@code BytesRef} object is created from the provided {@code String} value using UTF-8 encoding. As a result, its length
+ * can exceed that of the {@code String} value, since Java strings use UTF-16 encoding. This necessitates appropriate truncation.
+ *
+ * <p>Multi-byte sequences will be of the form {@code 11xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx}.
+ * The method first truncates continuation bytes, which start with {@code 10} in binary. It then truncates the head byte, which
+ * starts with {@code 11}. Both truncation operations use a binary mask of {@code 11100000}.
Review Comment:
You are right the comment essentially mentions the wrong mask.
> I think this operation is complex enough that I'd be in strong favour of refactoring it into its own function and documented accordingly.
I am not in favor of refactoring it as the operation is atomic and really does one thing which is to get a bytesRef. But then if you feel strongly let's take the refactoring as a separate issue.
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
+ * of the {@code BytesRef} object to the specified {@code maxLength}, ensuring that the multi-byte sequences are
+ * properly truncated.
+ *
+ * <p>The {@code BytesRef} object is created from the provided {@code String} value using UTF-8 encoding. As a result, its length
+ * can exceed that of the {@code String} value, since Java strings use UTF-16 encoding. This necessitates appropriate truncation.
+ *
+ * <p>Multi-byte sequences will be of the form {@code 11xxxxxx 10xxxxxx 10xxxxxx 10xxxxxx}.
+ * The method first truncates continuation bytes, which start with {@code 10} in binary. It then truncates the head byte, which
+ * starts with {@code 11}. Both truncation operations use a binary mask of {@code 11100000}.
Review Comment:
You are right the comment essentially mentions the wrong mask.
> I think this operation is complex enough that I'd be in strong favour of refactoring it into its own function and documented accordingly.
I am not in favor of refactoring it as the operation is atomic and really does one thing which is to get a bytesRef. But then if you feel strongly let's take the refactoring as a separate issue.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] steffenvan commented on a diff in pull request #1071: OAK-10384: Fix stripping of large indexed ordered properties
Posted by "steffenvan (via GitHub)" <gi...@apache.org>.
steffenvan commented on code in PR #1071:
URL: https://github.com/apache/jackrabbit-oak/pull/1071#discussion_r1319553330
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -316,6 +314,58 @@ protected boolean indexTypeOrderedFields(Document doc, String pname, int tag, Pr
return fieldAdded;
}
+ /**
+ * Returns a {@code BytesRef} object constructed from the given {@code String} value and also truncates the length
Review Comment:
I think we could improve the comment here a little, as it essentially it says: "truncates the length to ensure that the multi-byte sequences are properly truncated". I understand that it does some truncation. But I'm still left with the question of why this is necessary, and in which scenarios? What are some examples of 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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org