Let Lucene remove floats and lone-surrogate strings from LIST and SET keys

docs/changelog.md

@@ -377,6 +377,33 @@ stores and orders above positive infinity, also finds what it does without the i
 NaN, `lt` and `gt` of NaN nothing, and `neq` of any other value finds NaN as well. A raw range reaches NaN by a NaN
 bound, as in `score:[5 TO NaN]`.
 
+##### Lucene no longer keeps the floats, and the strings with unpaired surrogates, removed from `LIST` and `SET` keys
+
+On a Lucene mixed index, most floats (`Float`) removed from a key of `LIST` or `SET` cardinality stayed in the
+element's document, as did the float which such a key of an edge held before it was set to another one, and `eq` and
+the ranges kept finding the element by them: after `g.V(v).properties("weights").drop()`, `g.V().has("weights", 0.1f)`
+and `g.V().has("weights", gt(0f))` still found `v`. Lucene stores a float as the double which it widens to, and
+compared the removed float with the stored values by its own decimal string, which differs from the double's for most
+floats: `0.1` against `0.10000000149011612`. Floats whose strings are the same, such as `1.5f` or `-2.5f`, were
+removed. A string with an unpaired surrogate stayed likewise, whatever its mapping, as Lucene stores U+FFFD in place of
+the surrogate. Lucene now compares a removed value with the stored ones in the form in which it stores them. What it
+writes doesn't change. A string longer than 16,000 characters is removed too where storage gives it back with its
+unpaired surrogates, as it does from 1.2.0 on (see "Strings longer than 16,000 characters keep their unpaired
+surrogates"); one which an earlier version stored comes back with a question mark in place of each unpaired surrogate,
+and stays in the index when it's removed.
+
+In exchange, a `SET` key mapped as `STRING` or `TEXTSTRING` which holds two strings that Lucene stores alike, which
+differ only in an unpaired surrogate, or in one and U+FFFD, holds one entry for both, whichever came first, so once one
+of them is removed, the other isn't found either. Two instants of one millisecond share one entry in the same way, as
+before.
+
+What an earlier version left in an index stays there: the removed values, and a second copy of a value which a `SET`
+key held when the value was set again, so that removing that value now takes out one of the two. A reindex writes the
+document of each element which holds a value of the index anew, without them, but leaves the document of an element
+which holds none as it is. To clear those too, stop the instances, delete the directory of each mixed index concerned,
+`index.[X].directory/<index name>`, and reindex those indexes. They stay enabled meanwhile, so until the reindex has
+finished, their queries miss the elements which it hasn't written yet.
+
 ##### Ranges and not-equals of a `TEXTSTRING` key compare its strings whole
 
 A key mapped as `TEXTSTRING` holds each string whole in a field of its own, which the index sorts the key by, and its

janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java

@@ -706,6 +706,118 @@ public void testListDeleteAddInOneTransaction() {
         assertCount(0, tx.query().has("alias", Text.CONTAINS, "Marko").vertices());
     }
 
+    //A float removed from a key of several values is no longer found, also one whose decimal string changes when it is
+    //widened to a double, as Lucene stores it: 0.1f as 0.10000000149011612
+    @ParameterizedTest
+    @ValueSource(strings = {"LIST", "SET"})
+    public void testRemovedFloatsOfAKeyOfSeveralValuesAreNotFound(Cardinality cardinality) {
+        if (!indexFeatures.supportsCardinality(cardinality)) return;
+
+        final PropertyKey weights = mgmt.makePropertyKey("weights").dataType(Float.class).cardinality(cardinality)
+            .make();
+        final PropertyKey kind = makeKey("kind", String.class);
+        makeLabel("knows");
+        mgmt.buildIndex("mixed", Vertex.class).addKey(weights).buildMixedIndex(INDEX);
+        mgmt.buildIndex("edges", Edge.class).addKey(weights).addKey(kind, getStringMapping()).buildMixedIndex(INDEX);
+        finishSchema();
+
+        //Each float but 1.5f has a decimal string other than the double's which it widens to
+        final float[] removed = {0.1f, -0.3f, 3.14f, Float.MIN_VALUE, Float.MAX_VALUE, 1.5f};
+        final JanusGraphVertex some = tx.addVertex();
+        final JanusGraphVertex none = tx.addVertex();
+        for (final float value : removed) {
+            some.property(cardinality.convert(), "weights", value);
+            none.property(cardinality.convert(), "weights", value);
+        }
+        some.property(cardinality.convert(), "weights", -2.5f);
+        //The float of an edge is replaced, and that of another edge removed, which keeps its document by its kind
+        final Object replaced = some.addEdge("knows", none, "weights", 0.1f).id();
+        final Object cleared = some.addEdge("knows", none, "weights", 0.1f, "kind", "k").id();
+        clopen();
+
+        GraphTraversalSource g = tx.traversal();
+        for (final float value : removed) {
+            g.V(some.id(), none.id()).properties("weights").hasValue(value).drop().iterate();
+        }
+        g.E(replaced).property("weights", 3.14f).iterate();
+        g.E(cleared).properties("weights").drop().iterate();
+        clopen(option(FORCE_INDEX_USAGE), true);
+
+        g = tx.traversal();
+        for (final float value : removed) {
+            assertFalse(g.V().has("weights", value).hasNext(), Float.toString(value));
+            assertEquals(0L, g.V().has("weights", value).count().next(), Float.toString(value));
+        }
+        assertEquals(Collections.singletonList(some.id()), g.V().has("weights", -2.5f).id().toList());
+        //A range of every finite float doesn't find the vertex whose floats were all removed
+        assertEquals(Collections.singletonList(some.id()), g.V().has("weights", P.gte(-Float.MAX_VALUE)).id().toList());
+        assertEquals(1L, g.V().has("weights", P.gte(-Float.MAX_VALUE)).count().next());
+        assertFalse(g.E().has("weights", 0.1f).hasNext());
+        assertEquals(0L, g.E().has("weights", 0.1f).count().next());
+        assertEquals(Collections.singletonList(replaced), g.E().has("weights", 3.14f).id().toList());
+        assertEquals(Collections.singletonList(replaced), g.E().has("weights", P.gte(-Float.MAX_VALUE)).id().toList());
+        assertEquals(1L, g.E().has("weights", P.gte(-Float.MAX_VALUE)).count().next());
+        assertEquals(Collections.singletonList(cleared), g.E().has("kind", "k").id().toList());
+    }
+
+    //A removed string of a key of several values is no longer found, also one with an unpaired surrogate, which UTF-8
+    //has no encoding for: Lucene stores U+FFFD in its place
+    @ParameterizedTest
+    @ValueSource(strings = {"LIST", "SET"})
+    public void testRemovedStringsWithUnpairedSurrogatesOfAKeyOfSeveralValuesAreNotFound(Cardinality cardinality) {
+        if (!indexFeatures.supportsCardinality(cardinality)) return;
+
+        final List<Object[]> keys = new ArrayList<>();
+        for (final Object[] key : new Object[][]{{"defaultString", Mapping.DEFAULT}, {"text", Mapping.TEXT},
+                {"string", Mapping.STRING}, {"textString", Mapping.TEXTSTRING}}) {
+            if (key[1] == Mapping.DEFAULT || indexFeatures.supportsStringMapping((Mapping) key[1])) {
+                keys.add(key);
+            }
+        }
+        final JanusGraphManagement.IndexBuilder index = mgmt.buildIndex("mixed", Vertex.class);
+        for (final Object[] key : keys) {
+            index.addKey(mgmt.makePropertyKey((String) key[0]).dataType(String.class).cardinality(cardinality).make(),
+                ((Mapping) key[1]).asParameter());
+        }
+        index.buildMixedIndex(INDEX);
+        finishSchema();
+
+        final String removed = "Ab\uD800c Removed";
+        final JanusGraphVertex some = tx.addVertex();
+        final JanusGraphVertex none = tx.addVertex();
+        for (final Object[] key : keys) {
+            some.property(cardinality.convert(), (String) key[0], removed);
+            some.property(cardinality.convert(), (String) key[0], "Kept");
+            none.property(cardinality.convert(), (String) key[0], removed);
+        }
+        clopen(option(FORCE_INDEX_USAGE), true);
+        assertStringsFound(keys, "removed", "Ab", Sets.newHashSet(some.id(), none.id()));
+
+        for (final Object[] key : keys) {
+            tx.traversal().V(some.id(), none.id()).properties((String) key[0]).hasValue(removed).drop().iterate();
+        }
+        clopen(option(FORCE_INDEX_USAGE), true);
+        assertStringsFound(keys, "removed", "Ab", Collections.emptySet());
+        assertStringsFound(keys, "kept", "Kept", Collections.singleton(some.id()));
+    }
+
+    //The vertices found by a word, on a key mapped to the words of its strings, and by a prefix, on a key mapped to its
+    //whole strings: eq of a string with an unpaired surrogate finds nothing on Elasticsearch and Solr
+    private void assertStringsFound(List<Object[]> keys, String word, String prefix, Set<Object> expected) {
+        final GraphTraversalSource g = tx.traversal();
+        for (final Object[] key : keys) {
+            final String name = (String) key[0];
+            if (key[1] != Mapping.STRING) {
+                assertEquals(expected, g.V().has(name, Text.textContains(word)).id().toSet(), name);
+                assertEquals((long) expected.size(), g.V().has(name, Text.textContains(word)).count().next(), name);
+            }
+            if (key[1] == Mapping.STRING || key[1] == Mapping.TEXTSTRING) {
+                assertEquals(expected, g.V().has(name, Text.textPrefix(prefix)).id().toSet(), name);
+                assertEquals((long) expected.size(), g.V().has(name, Text.textPrefix(prefix)).count().next(), name);
+            }
+        }
+    }
+
     @Test
     public void testIndexing() throws InterruptedException {
 

janusgraph-lucene/src/main/java/org/janusgraph/diskstorage/lucene/LuceneIndex.java

@@ -459,7 +459,10 @@ private void removeFieldIfNeeded(Document doc, String fieldName, Object fieldVal
     private String convertToStringValue(final Mapping mapping, Object value) {
         String converted;
         if (value instanceof Number) {
-            converted = value.toString();
+            //As buildStoreField stores it: a float as the double which it widens to, whose decimal string differs from
+            //the float's for most floats, such as 0.10000000149011612 for 0.1f
+            converted = AttributeUtils.isWholeNumber((Number) value) ? String.valueOf(((Number) value).longValue())
+                : String.valueOf(((Number) value).doubleValue());
         } else if (AttributeUtils.isString(value)) {
             if (mapping == Mapping.DEFAULT || mapping == Mapping.TEXT || mapping == Mapping.TEXTSTRING) {
                 converted = lowerCaseCodePoints((String) value);
@@ -477,7 +480,8 @@ private String convertToStringValue(final Mapping mapping, Object value) {
         } else {
             throw new IllegalArgumentException("Unsupported type: " + value);
         }
-        return converted;
+        //Lucene stores a string in UTF-8, with U+FFFD in place of each unpaired surrogate
+        return AttributeUtils.isString(value) ? new BytesRef(converted).utf8ToString() : converted;
     }
 
     // NOTE: new SET/LIST store fields must be sync with convertToStringValue