Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
codePoints().toArray()allocates two arrays per string compare, which can become a significant… · New The descending-path scan walks the full string and may allocate a very large intermediate… · New This test performs 100,000 iterations and repeatedly encodes strings to UTF-8 bytes… · New The comment says “The values in their UTF-16 units”, but the implementation operates on… · New
What changed in this PR
This PR improves how JanusGraph narrows mixed-index reads for ordered traversals inside a transaction when in-tx mutations fill the query limit, expanding support to more valid UTF-16 strings (including beyond U+D800) and TEXTSTRING mappings, while also preventing backend failures from overly long string range bounds.
Changes:
- Extend ordered-range narrowing eligibility from
STRINGtoTEXTSTRING(when whole-string comparison is safe) and from “below U+D800” to any valid UTF-16 without unpaired surrogates. - Introduce
rangeBound(...)to safely derive range bounds (cuts descending bounds at U+FFFD and enforces a 1000 UTF-8 byte limit; disables ascending bounds beyond the limit). - Add/expand unit + backend tests and update docs/changelog to describe the new behavior and limitations.
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java | Core logic changes: broaden ranged-string eligibility and introduce bounded/safe range bound derivation. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/query/QueryExecutor.java | Javadoc clarifies that narrowing may use a bound beyond the provided value in some cases. |
| janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTxOrderRangeTest.java | New/expanded unit tests for valid UTF-16 strings, surrogate handling, and bound-cutting behavior. |
| janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java | New/updated integration tests validating range narrowing across backends and new TEXTSTRING behavior. |
| docs/changelog.md + docs/basics/transactions.md | Document behavior changes, limitations, and direct index query semantics. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4bd1a48 to
60eb395
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
This comparator allocates two newint[]arrays per string comparison (codePoints().toArray()),… · New This randomized test does 100k iterations and, in many cases, performs UTF-8 encoding work… · NewByteBuffer.array()is not guaranteed to be supported for all ByteBuffer implementations (it can… · NewrangeBound(value, descending)is computed before checkingindexSerializer.supports(...). Since… · New The statement “a byte which a URL escapes takes three characters” appears to understate worst-case… · New
4 resolved since last review
This test performs 100,000 iterations and repeatedly encodes strings to UTF-8 bytes… The descending-path scan walks the full string and may allocate a very large intermediate…codePoints().toArray()allocates two arrays per string compare, which can become a significant… The comment says “The values in their UTF-16 units”, but the implementation operates on…
🧠 Review effort: Lite
60eb395 to
dade9c4
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
This comparator allocates two int arrays per string comparison (codePoints().toArray()), which… · New The javadoc contradicts itself: it says 'a string without a surrogate out of its pair' (which is… · New This comment claims the descending bound becomes 'prefix + U+FFFD' whenever there is a character >=… · New
5 resolved since last review
ByteBuffer.array()is not guaranteed to be supported for all ByteBuffer implementations (it can… This randomized test does 100k iterations and, in many cases, performs UTF-8 encoding work… This comparator allocates two newint[]arrays per string comparison (codePoints().toArray()),… The statement “a byte which a URL escapes takes three characters” appears to understate worst-case…rangeBound(value, descending)is computed before checkingindexSerializer.supports(...). Since…
🧠 Review effort: Lite
…d TEXTSTRING keys Fixes JanusGraph#5084 An ordered graph query in a transaction whose new elements fill its limit reads from the mixed index only the elements whose value of the first order key doesn't come after that of the last of them within the limit, where every index compares that value as memory orders it. For a string, StandardJanusGraphTx.isRangedAsOrdered took the range only below U+D800 and for a key mapped as STRING. Since JanusGraph#5067, memory orders strings by code point, as Elasticsearch, OpenSearch, Lucene and Solr do, and since JanusGraph#5069, the ranges of a TEXTSTRING key compare its whole strings in the field which the index sorts it by, on every index backend. So a query whose last new value had a character from U+E000 or from U+10000, or whose key was mapped as TEXTSTRING, read the index past every element the transaction had moved: after renaming 20,000 of 50,000 vertices to names from U+E000, order().by("name", desc).limit(10) ran 155 Elasticsearch searches in 1.2 s, where one search in 90 ms does. The range is now taken for any string without a surrogate out of its pair, on a field mapped as STRING or TEXTSTRING without a string or text analyzer. A surrogate without its pair isn't valid UTF-16, and no index holds it as it is: Lucene, Solr, OpenSearch and Elasticsearch up to 8 hold U+FFFD, Elasticsearch 9.5 a question mark, while memory places it after U+FFFF in a stored string which storage keeps as written, as it keeps one of up to 16,000 UTF-16 units. Where the index holds U+FFFD, such a stored string may come below a valid value while memory places it above, where the value has a character from U+FFFD up at the first unit in which they differ. An ascending range holds such a string anyway, as it comes after the value. A descending range from such a value now starts at the value's units before that character followed by U+FFFD, which the index places no such string below (rangeBound). JanusGraph's Lucene backend, OpenSearch 2 and Elasticsearch 6 to 8 can't build a range from a lower bound of more than 1,000 UTF-8 bytes ("input automaton is too large"), and Solr takes a query in a URL of at most 8 KB, as JanusGraph sends its queries to Solr as GET requests. So a descending query whose last new value was longer failed on the former with "Could not call index", and on Solr a query in either direction whose value had about 8 KB, also on master for a STRING key below U+D800. A descending bound is now cut to its longest prefix of whole characters within 1,000 bytes, a lower bound still, and no ascending range is taken from a value of more than 1,000 bytes, as an upper bound can't be cut. Two cases are left as on master, where ranges of strings below U+D800 show them too. Elasticsearch 9.5 places such a surrogate below the letters, so its descending range from nearly any string may leave out a stored string with one. And where the index holds U+FFFD, an ascending range may leave out a stored string of more than 16,000 units which storage holds with a question mark in place of such a surrogate, as it does where it compresses the string from its UTF-8 bytes. The transactions page now also says that direct index queries read the index alone: they leave out the elements which the transaction removed, but its other changes don't change what they find. Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
dade9c4 to
99cbbdb
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
codePoints().toArray()allocates anint[]on every comparison, which can be a significant cost… · New This helper assumes the returnedByteBufferis array-backed (bytes.array()), which is not… · New IntheRangeHoldsEveryStoredStringWhichMemoryOrdersUpToTheValuethis method can be called a large… · New The doc states the derived descending bound is the prefix followed by U+FFFD, but the subsequent… · New
3 resolved since last review
🧠 Review effort: Lite
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
4 resolved since last review
IntheRangeHoldsEveryStoredStringWhichMemoryOrdersUpToTheValuethis method can be called a large… This helper assumes the returnedByteBufferis array-backed (bytes.array()), which is not…codePoints().toArray()allocates anint[]on every comparison, which can be a significant cost… The doc states the derived descending bound is the prefix followed by U+FFFD, but the subsequent…
Review effort: Lite
| final int[] first = ((String) a).codePoints().toArray(); | ||
| final int[] second = ((String) b).codePoints().toArray(); | ||
| for (int i = 0; i < Math.min(first.length, second.length); i++) { | ||
| if (first[i] != second[i]) { | ||
| return Integer.compare(first[i], second[i]); | ||
| } | ||
| } | ||
| return Integer.compare(first.length, second.length); |
| private static byte[] indexed(String string) throws CharacterCodingException { | ||
| final ByteBuffer bytes = StandardCharsets.UTF_8.newEncoder().onMalformedInput(CodingErrorAction.REPLACE) | ||
| .replaceWith(new byte[]{(byte) 0xEF, (byte) 0xBF, (byte) 0xBD}).encode(CharBuffer.wrap(string)); | ||
| return Arrays.copyOf(bytes.array(), bytes.limit()); |


Fixes #5084
An ordered graph query which a mixed index sorts, in a transaction whose new or changed elements fill the query's limit, reads from the index only the elements whose value of the first order key doesn't come after that of the last of them within the limit (#5023). For a string, it took that range only below U+D800 and only for a key mapped as
STRING. Since #5067, memory orders strings by code point, as the indexes do, and since #5069, the ranges of aTEXTSTRINGkey compare its whole strings on every index backend. This PR takes the range for any string without a surrogate out of its pair, and for aTEXTSTRINGkey as well, without a string or text analyzer either way. After a transaction renamed 20,000 of 50,000 vertices to names from U+E000,order().by("name", desc).limit(10)runs one Elasticsearch search in 90 ms, instead of 155 in 1.2 s.It also fixes failures of #5023's ranges from long values:
STRINGkey below U+D800.A descending bound is now cut to its longest prefix of whole characters within 1,000 bytes, and no ascending range is taken from a value of more than 1,000 bytes.
A surrogate without its pair isn't valid UTF-16, and no index holds it as it is: Lucene, Solr, OpenSearch and Elasticsearch up to 8 hold U+FFFD, and Elasticsearch 9.5 holds a question mark, while memory places it after U+FFFF. So a descending range of a string with a character from U+FFFD up, such as an emoji, starts at U+FFFD in place of that character and what follows it. Where the index holds U+FFFD, a descending query then returns what it returns without the range, also where the index holds such strings. On Elasticsearch 9.5, a descending range from nearly any string may leave such a stored string out, as master's ranges of strings below U+D800 already may.
Changes
StandardJanusGraphTx.isRangedAsOrderedtakes a range for a string without a surrogate out of its pair (hasUnpairedSurrogate, new), in place of one below U+D800 (isBelowTheSurrogates). Memory and the indexes compare valid strings alike: by code point, the order of their UTF-8 bytes.isComparedWholetakes a field mapped asTEXTSTRINGas well asSTRING. ATEXTSTRINGkey's ranges and its order read the field of its whole strings on each backend:<key>_____s. The keyword analyzer makes its one term the whole string, and its sort doc values hold the string as it is. Its ranges read it since Compare the whole strings of a TEXTSTRING key in ranges and not-equals #5069.<key>__STRING, akeywordfield. Its ranges read it since Compare the whole strings of a TEXTSTRING key in ranges and not-equals #5069.dyn-fields, the default),<key>_s, the field of aSTRINGkey of the same name, or<mapped name>_s. The Solr schema defines it: the schema JanusGraph ships makes*_saStrField, which keeps the string whole. Its ranges always read it; Compare the whole strings of a TEXTSTRING key in ranges and not-equals #5069 changed only its not-equal.rangeBound(new) gives the bound of the range, or none:rangeBounddoesn't try.IllegalArgumentException: input automaton is too large: 1001(Lucene 9'sOperations.isFinite).SolrRangeQuery), but JanusGraph sends its queries to Solr as GET requests, and Solr 9.10 takes at most 8 KB of request line and headers.ClientUtils.escapeQueryCharsand the URL make at most six characters of a byte, so a bound of 1,000 bytes takes at most 6,000.QueryExecutor.withoutResultsAfter: its javadoc says that the narrowed sub-query may keep some results after the value, those up to a bound after it.graph.indexQuery()or a transaction'sindexQuery()) read the index alone. They leave out the elements which the transaction removed, but its other changes don't change what they find.IndexQueryBuilder.executereads the index, and leaves out only the ids which turn into a removed element or into none.executeTotalscounts what the index holds.Tests
StandardJanusGraphTxOrderRangeTest(janusgraph-test, 11 tests, 6 before):takesARangeForAStringOfValidUtf16(new): U+D7FF, U+E000, U+FFFD, U+FFFF, U+1F600, U+10000, U+10FFFF and strings of them, onSTRINGandTEXTSTRINGfields, both ways.takesNoRangeForAStringWithASurrogateOutOfItsPair: a lone high or low surrogate, at the end or before another character, a reversed pair, and a high surrogate before a pair.takesARangeForAStringOnlyOnAFieldWhichComparesItWhole:STRINGandTEXTSTRING, neither with a string or text analyzer, and notTEXTor the default mapping.cutsTheDescendingBoundOfAStringBeforeItsFirstCharacterFromUFFFDUp(new): the bounds of both directions.cutsTheDescendingBoundOfALongStringToItsFirstThousandUtf8Bytes(new): 1,001w, 400 U+4E2D and 400 U+E000, a two-byte character which would end at byte 1,001, and the U+FFFD in place of an emoji at either side of the limit.takesNoAscendingRangeFromAStringOfMoreThanAThousandUtf8Bytes(new): 1,000w, 333 U+4E2D and 996afollowed by an emoji take an ascending range whole; 1,001w, 334 U+4E2D, 999afollowed by a two-byte character, and 997afollowed by an emoji take none.theRangeHoldsEveryStoredStringWhichMemoryOrdersUpToTheValue(new): 100,000 random pairs of a value and a stored string, of characters from both sides of the surrogates and of surrogates on their own. WhereverOrderListplaces the stored string at or before the value, a model of an index which holds U+FFFD places it within the range from the bound: UTF-8 bytes, with U+FFFD for a surrogate without its pair.JanusGraphIndexTest, on every index backend. The mixed indexes are namedmixed, which the Solr test container creates.testTransactionReadsTheIndexOnlyUpToItsLastNewValueWithinTheLimit: its value which an index may compare otherwise is now a lone U+D800, as U+E000 takes the range (see the next test), and an emoji's descending range starts at U+FFFD. The results are as before.testTransactionReadsTheIndexUpToAStringBeyondTheSurrogatesInCodePointOrder(new): stored U+FFFF and U+1F601, new U+E000 and U+1F600.testTransactionRangeKeepsTheResultsOfADescendingOrderWithAStoredUnpairedSurrogate(new): storeda, a lone U+DBFF, and a lone U+DC00 followed byb. With U+FFFE, and with U+1F600, added in a transaction, the descending range oflimit(1)starts at U+FFFD, andlimit(1)returns what the query without a limit returns first.b, which memory places before both new strings.a.testTransactionRangesFromALastNewValueOfMoreThanAThousandBytes(new): aSTRINGkey and aTEXTSTRINGkey, a stored value of 1,000wfollowed byx, and a storeda. The transaction adds a value of 1,000w, of 1,001w, of 400 U+4E2D or of 400 U+E000.wtake it whole.limit(1)in either direction returns what the query without a limit returns first.testTransactionReadsTheIndexOfADualMappedKeyOnlyUpToItsLastNewValueWithinTheLimit(new, whereTEXTSTRINGis supported): titles of several words in mixed case. The ranges up toLong Zedandzz top 2, and fromzz top, are taken and give the right titles. A range of the text field's lower-cased words would find no stored title up toLong Zed, and leave out~Tilde Titlefromzz top.testTransactionReadsTheIndexWholeWhereItMayCompareOtherwise: itsTEXTSTRINGcase moves to the new test above. A new value of a lone U+D800 takes no range either way, and the results are right.testTransactionQueriesAgreeWithAScanOfTheirElements: the strings now include U+E000, U+FFFD, U+FFFF and emoji, and a newTEXTSTRINGorder key holds words in mixed case, also from both sides of the surrogates. The scan orders strings by code point. It runs 20 seeds, as before.StandardJanusGraphTxOrderRangeTestand the Lucene index tests:TEXTSTRINGquery.wdoes.w. On Solr, a range from 8 KB fails, as above.a;limit(1)of the stored-surrogate test returns U+FFFE where the query without a limit returns U+DC00bfirst.bfirst.TEXTSTRINGtake no range, and five Lucene tests on the missing range.TEXTSTRINGleft out: two unit tests fail, and two Lucene tests on theTEXTSTRINGkey's missing range: the dual-mapped test and the long-value test.takesNoRange...fails on U+D800;...WhereItMayCompareOtherwiseand...OnlyUpToItsLastNewValueWithinTheLimittake a range from the lone U+D800.takesARangeForAStringOnlyOnAFieldWhichComparesItWholefails.STRINGkey: 36 cut descending, 5 descending uncut and 22 ascending;TEXTSTRINGkey: 28 cut descending and 8 ascending.Verification
Locally, Java 11, on master
3285cbf18, with checkstyle (no violations):BerkeleyLuceneTest108) pass.BerkeleyElasticsearchTest(121, 1 skipped) passes against Elasticsearch 9.5.4 in a container.BinaryRequestWriteris the default ofHttpSolrClientandCloudLegacySolrClient), whose encoder writes U+FFFD for a surrogate without its pair, as Lucene'sBytesRefdoes.\uescape.gterange from 1,001 bytes, and build anlterange up to 1,001 bytes.gteup to 1,000 bytes, for characters of one to four bytes, andlteup to 40,000 bytes, but not up to 1,000 NUL characters followed by more.name_srange from 7,000 ASCII characters in either direction. A GET request with one from 8,200 fails with 414 URI Too Long, and one from 900 U+4E2D with 431 Request Header Fields Too Large; a POST takes one from 20,000.Measurements
Elasticsearch 9.5.4, a single node in Docker, inmemory storage. A mixed index holds
kind,name(STRING) andtitle(TEXTSTRING) of 50,000 vertices ofkindk, namedn00000ton49999and titledTitle 00000toTitle 49999. A transaction renames, or retitles, the 20,000 vertices with the highest names, andg.V().has("kind", "k").order().by(key, desc).limit(10)runs in it.How it was measured:
query_totalof the graph's indexes.TEXTSTRING):zz, a space and the numberzz, a space and the number (control)name >= U+E000 49990,name >= U+FFFD(cut fromU+1F600 49990) andtitle >= zz 49990.Notes
b, andzadded in a transaction, master'sorder().by("name", desc).limit(1)takes the rangename >= zand returnsz, while the query without a limit returns U+DC00bfirst. On Lucene, master returns U+DC00b.x, andaadded in a transaction, master'sorder().by("name", asc).limit(1)on Lucene takesname <= aand returnsa, while the query without a limit returns the stored string, read back as?xx…, first. Keep the unpaired surrogates of strings longer than 16,000 characters #5098 makes storage keep such surrogates in long strings. With it, this case remains only for strings written before it.TEXTSTRINGkey as for aSTRINGkey since Find the vertices a transaction changed for its graph queries by key and value #5023, and the order of either. The test schema does so forfull_text_s, which belongs to a key with English string and text analyzers, so it takes no range. The changelog says so too.TEXTSTRINGkey with a text analyzer only could take the range too, as that analyzer doesn't shape the field of its whole strings on Lucene or Elasticsearch. It is left out as before.🤖 Generated with Claude Code