search

JanusGraph / janusgraph

JanusGraph: an open-source, distributed graph database
https://janusgraph.org
Other
5.32k stars 1.17k forks source link

Inmemory store does not support values more than 127 bytes in size for properties with Cardinality.SET (and doesn't support key sizes > 127 bytes) #2273

Closed mad closed 3 years ago

mad commented 3 years ago

For confirmed bugs, please report:

For berkeleyje storage works as expected

graph = JanusGraphFactory.open("berkeleyje:/tmp/graph")

mgmt = graph.openManagement()
p1 = mgmt.makePropertyKey('p1').cardinality(Cardinality.SET).dataType(String.class).make()
mgmt.buildIndex("idx", Vertex.class).addKey(p1).buildCompositeIndex();
mgmt.commit();

v1=graph.traversal().addV("Portrait").property("p1", '-'*125).next()
graph.tx().commit()

But for inmemory store failed on commit

graph = JanusGraphFactory.open("inmemory")

mgmt = graph.openManagement()
p1 = mgmt.makePropertyKey('p1').cardinality(Cardinality.SET).dataType(String.class).make()
mgmt.buildIndex("idx", Vertex.class).addKey(p1).buildCompositeIndex();
mgmt.commit();

v1=graph.traversal().addV("Portrait").property("p1", '-'*125).next()
graph.tx().commit()

with exception

...
Caused by: java.lang.IllegalArgumentException
    at com.google.common.base.Preconditions.checkArgument(Preconditions.java:108)
    at org.janusgraph.diskstorage.inmemory.BufferPageUtils.buildFromEntryArray(BufferPageUtils.java:110)
    at org.janusgraph.diskstorage.inmemory.BufferPage.merge(BufferPage.java:236)
    at org.janusgraph.diskstorage.inmemory.SinglePageEntryBuffer.merge(SinglePageEntryBuffer.java:36)
    at org.janusgraph.diskstorage.inmemory.SinglePageEntryBuffer.mutate(SinglePageEntryBuffer.java:83)
    at org.janusgraph.diskstorage.inmemory.InMemoryColumnValueStore.mutate(InMemoryColumnValueStore.java:128)
    at org.janusgraph.diskstorage.inmemory.InMemoryKeyColumnValueStore.mutate(InMemoryKeyColumnValueStore.java:106)
    at org.janusgraph.diskstorage.inmemory.InMemoryStoreManager.mutateMany(InMemoryStoreManager.java:127)
    at org.janusgraph.diskstorage.locking.consistentkey.ExpectedValueCheckingStoreManager.mutateMany(ExpectedValueCheckingStoreManager.java:79)
    at org.janusgraph.diskstorage.keycolumnvalue.cache.CacheTransaction$1.call(CacheTransaction.java:91)
    at org.janusgraph.diskstorage.keycolumnvalue.cache.CacheTransaction$1.call(CacheTransaction.java:88)
    at org.janusgraph.diskstorage.util.BackendOperation.executeDirect(BackendOperation.java:68)
li-boxuan commented 3 years ago

I can reproduce it on 0.5.2.

porunov commented 3 years ago

@dk-github Do you know why it may happen?

dk-github commented 3 years ago

We routinely store strings hundreds of MBs in size, so this looks to be a "special" case which wasn't covered in any of the tests or use cases so far and so went unnoticed.

I had a quick look, and the above code works fine if we remove the following: cardinality(Cardinality.SET)

Also, the comment above the failing precondition line says that it assumes the length of the key (not value) in an Entry is always less than 127, and uses that assumption to reduce the overhead (1 byte to store the position where value starts instead of 4 bytes).

So my hypothesis (not based on actual code dive so far) is that normally JG stores property as value under a key equal to property name, but in case of Cardinality.SET it switches to a structure which effectively stores values as keys (and thus achieves uniqueness).

Summary (NOTE that it is actually premature because it would be great to verify by diving in the code - will try doing this later next week and update this post if not correct):

The options I can see here are: a) remove this optimization and always use full 4-byte integers for indexing - this will resolve the issue, but would be sad, because 99.99% of Entries is going to be 3 bytes longer than they actually need to be, and with big graphs this can be many hundreds of MB, possibly a few GBs

b) invest some time in an "adaptive" storage strategy, so that in 99.99% cases it uses 1-byte indexes, and switches to 4-byte only if required, on per-page or per-entry basis. For example, use sign bit in first byte to signify if it is a 1-byte or 4-byte length. We already do a similar thing where 99% of buffers are single-page and avoid the overhead of embedded page list etc, and we only switch to multi-page if the size of the data dictates it, which is relatively seldom

c) check if there is a practical limit to the key size in other backends, and if so - should we document these limits as limitations of Cardinality.SET, or look for a more involved but less limited implementation of SET on frontend side

I will try to set aside some time to verify the hypothesis, and look at how difficult would option b be. In the meantime, any thoughts are welcome.

dk-github commented 3 years ago

I opened a PR https://github.com/JanusGraph/janusgraph/pull/2294 which implements option b) - i.e. it uses 1 byte to store key length unless it exceeds 127, in which case it switches to full 4-byte integer and stores it negated. SO that by reading 1st byte and checking if it is negative it can then detect if it needs to read 3 more. It relies on BigEndian order being default in java.nio cross-platform.

I haven't made any performance or memory footprint comparisons yet, but I expect the impact to be negligible to both.

Please review.

I have also converted the repro code above to Java, not sure if we want to make it into a test specifically for storing big values with Cardinality.SET - and where to put that test if we do - so just posting it here for now:

package org.janusgraph.diskstorage.inmemory;

import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.janusgraph.core.Cardinality;
import org.janusgraph.core.JanusGraph;
import org.janusgraph.core.JanusGraphFactory;
import org.janusgraph.core.PropertyKey;
import org.janusgraph.core.schema.JanusGraphManagement;
import org.junit.jupiter.api.Test;

import java.util.ArrayList;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.*;

class issue2273 {
    @Test
    void repro2273CardinalitySetLongValue() {
        JanusGraph graph = JanusGraphFactory.open("inmemory");

        StringBuffer longStringValue = new StringBuffer("-");
        for(int i=0; i < 130; i++)
        {
            longStringValue.append('-');
        }

        assertTrue(longStringValue.length() > 127); //original issue reproduces when value is 125 symbols or more

        JanusGraphManagement mgmt = graph.openManagement();
        PropertyKey p1 = mgmt.makePropertyKey("p1").cardinality(Cardinality.SET).dataType(String.class).make();
        PropertyKey p2 = mgmt.makePropertyKey("p2").dataType(String.class).make();
        PropertyKey p3 = mgmt.makePropertyKey(longStringValue.toString()).dataType(String.class).make();
        mgmt.buildIndex("idx", Vertex.class).addKey(p1).buildCompositeIndex();
        mgmt.commit();

        Vertex v1 = graph.traversal().addV("Portrait")
            .property("p1", longStringValue.toString()).property("p1", longStringValue.toString())
            .property("p2", longStringValue.toString())
            .property(longStringValue.toString(), longStringValue.toString())
            .next();
        graph.tx().commit();

        Map<Object, Object> vals = graph.traversal().V().valueMap().next();
        assertEquals(longStringValue.toString(), ((ArrayList)(vals.get("p2"))).get(0));
        assertEquals(longStringValue.toString(), ((ArrayList)(vals.get("p1"))).get(0));
        assertEquals(longStringValue.toString(), ((ArrayList)(vals.get(longStringValue.toString()))).get(0));
    }

}
li-boxuan commented 3 years ago

I have also converted the repro code above to Java, not sure if we want to make it into a test specifically for storing big values with Cardinality.SET - and where to put that test if we do - so just posting it here for now

It would be nice if you can put it in JanusGraphTest.java

porunov commented 3 years ago

Fixed in #2294