Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support null value for gremlin test #2061

Merged
merged 9 commits into from
Jan 16, 2023
4 changes: 2 additions & 2 deletions .github/workflows/stale.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ jobs:
stale-pr-message: 'Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label'
stale-issue-label: 'inactive'
stale-pr-label: 'inactive'
exempt-issue-labels: 'feature,bug,enhancement,improvement,wontfix,todo,guide,doc,help wanted'
exempt-pr-labels: 'feature,bug,enhancement,improvement,wontfix,todo,guide,doc,help wanted'
exempt-issue-labels: 'feature,bug,enhancement,improvement,todo,guide,doc,help wanted,security'
exempt-pr-labels: 'feature,bug,enhancement,improvement,todo,guide,doc,help wanted,security'
exempt-all-milestones: true

days-before-issue-stale: 15
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ private <V> boolean checkDataType(V value) {
}

/**
* Check type of all the values(may be some of list properties) valid
* Check type of all the values(maybe some list properties) valid
* @param values the property values to be checked data type
* @param <V> the property value class
* @return true if all the values are or can convert to the data type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@
import org.apache.hugegraph.backend.serializer.BytesBuffer;
import org.apache.hugegraph.perf.PerfUtil.Watched;
import org.apache.hugegraph.util.E;
import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyProperty;

import com.google.common.collect.ImmutableList;

public class HugeEdge extends HugeElement implements Edge, Cloneable {

private Id id;
private EdgeLabel label;
private final EdgeLabel label;
private String name;

private HugeVertex sourceVertex;
Expand Down Expand Up @@ -200,6 +202,11 @@ public <V> Property<V> property(String key, V value) {
E.checkArgument(this.label.properties().contains(propertyKey.id()),
"Invalid property '%s' for edge label '%s'",
key, this.label());
if (value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also update HugeVertex.property(String key, V value)

imbajin marked this conversation as resolved.
Show resolved Hide resolved
this.removeProperty(propertyKey.id());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if remove null value's property, may cause tinkerpop ut failed ?

Copy link
Member

@imbajin imbajin Dec 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow the tinkerpop ci action tests with release-1.0.0

return EmptyProperty.instance();
}

// Sort-Keys can only be set once
if (this.schemaLabel().sortKeys().contains(propertyKey.id())) {
E.checkArgument(!this.hasProperty(propertyKey.id()),
Expand Down Expand Up @@ -279,7 +286,7 @@ public <V> Iterator<Property<V>> properties(String... keys) {

if (keys.length == 0) {
for (HugeProperty<?> prop : this.getProperties()) {
assert prop instanceof Property;
assert prop != null;
props.add((Property<V>) prop);
}
} else {
Expand All @@ -295,7 +302,6 @@ public <V> Iterator<Property<V>> properties(String... keys) {
// Not found
continue;
}
assert prop instanceof Property;
props.add((Property<V>) prop);
}
}
Expand All @@ -320,8 +326,7 @@ public Object sysprop(HugeKeys key) {
case PROPERTIES:
return this.getPropertiesMap();
default:
E.checkArgument(false,
"Invalid system property '%s' of Edge", key);
E.checkArgument(false, "Invalid system property '%s' of Edge", key);
return null;
}
}
Expand Down Expand Up @@ -362,6 +367,7 @@ public void vertices(HugeVertex owner, HugeVertex other) {
if (ownerLabel.equals(this.label.sourceLabel())) {
this.vertices(true, owner, other);
} else {
// TODO: why compare the label but ignore the result?
ownerLabel.equals(this.label.targetLabel());
this.vertices(false, owner, other);
}
Expand Down Expand Up @@ -515,8 +521,7 @@ public static HugeEdge constructEdge(HugeVertex ownerVertex,
ownerVertex.correctVertexLabel(tgtLabel);
otherVertexLabel = srcLabel;
}
HugeVertex otherVertex = new HugeVertex(graph, otherVertexId,
otherVertexLabel);
HugeVertex otherVertex = new HugeVertex(graph, otherVertexId, otherVertexLabel);

ownerVertex.propNotLoaded();
otherVertex.propNotLoaded();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,19 @@
import org.apache.hugegraph.backend.id.EdgeId;
import org.apache.hugegraph.backend.id.Id;
import org.apache.hugegraph.backend.id.IdGenerator;
import org.apache.hugegraph.backend.serializer.BytesBuffer;
import org.apache.hugegraph.backend.tx.GraphTransaction;
import org.apache.hugegraph.perf.PerfUtil.Watched;
import org.apache.hugegraph.schema.PropertyKey;
import org.apache.hugegraph.schema.SchemaLabel;
import org.apache.hugegraph.schema.VertexLabel;
import org.apache.hugegraph.type.HugeType;
import org.apache.hugegraph.type.Idfiable;
import org.apache.hugegraph.type.define.Cardinality;
import org.apache.hugegraph.type.define.HugeKeys;
import org.apache.hugegraph.util.CollectionUtil;
import org.apache.hugegraph.util.E;
import org.apache.hugegraph.util.InsertionOrderUtil;
import org.apache.hugegraph.util.collection.CollectionFactory;
import org.apache.tinkerpop.gremlin.structure.Element;
import org.apache.tinkerpop.gremlin.structure.Property;
Expand All @@ -47,12 +52,6 @@
import org.eclipse.collections.api.iterator.IntIterator;
import org.eclipse.collections.api.map.primitive.MutableIntObjectMap;

import org.apache.hugegraph.backend.serializer.BytesBuffer;
import org.apache.hugegraph.perf.PerfUtil.Watched;
import org.apache.hugegraph.util.CollectionUtil;
import org.apache.hugegraph.util.E;
import org.apache.hugegraph.util.InsertionOrderUtil;

public abstract class HugeElement implements Element, GraphType, Idfiable {

private static final MutableIntObjectMap<HugeProperty<?>> EMPTY_MAP =
Expand All @@ -61,8 +60,8 @@ public abstract class HugeElement implements Element, GraphType, Idfiable {

private final HugeGraph graph;
private MutableIntObjectMap<HugeProperty<?>> properties;

private long expiredTime; // TODO: move into properties to keep small object
// TODO: move into properties to keep small object
private long expiredTime;

private boolean removed;
private boolean fresh;
Expand Down Expand Up @@ -425,14 +424,14 @@ public static final ElementKeys classifyKeys(Object... keyValues) {
Object val = keyValues[i + 1];

if (!(key instanceof String) && !(key instanceof T)) {
throw Element.Exceptions
.providedKeyValuesMustHaveALegalKeyOnEvenIndices();
throw Element.Exceptions.providedKeyValuesMustHaveALegalKeyOnEvenIndices();
}
if (val == null) {
if (key.equals(T.label)) {
if (T.label.equals(key)) {
throw Element.Exceptions.labelCanNotBeNull();
}
throw Property.Exceptions.propertyDoesNotExist();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems need to skip the null value here with continue, also add the comment for why

// Ignore null value for tinkerpop test compatibility
continue;
}
imbajin marked this conversation as resolved.
Show resolved Hide resolved

if (key.equals(T.id)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ public PropertyKey propertyKey() {
}

public Object id() {
return SplicingIdGenerator.concat(this.owner.id().asString(),
this.key());
return SplicingIdGenerator.concat(this.owner.id().asString(), this.key());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
Expand Down Expand Up @@ -59,6 +60,9 @@
import org.apache.hugegraph.type.define.HugeKeys;
import org.apache.hugegraph.type.define.IdStrategy;
import org.apache.hugegraph.util.E;
import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyProperty;
import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyVertexProperty;

import com.google.common.collect.ImmutableList;

public class HugeVertex extends HugeElement implements Vertex, Cloneable {
Expand Down Expand Up @@ -162,8 +166,7 @@ public void assignId(Id id, boolean force) {
}
break;
default:
throw new AssertionError(String.format(
"Unknown id strategy '%s'", strategy));
throw new AssertionError(String.format("Unknown id strategy '%s'", strategy));
}
this.checkIdLength();
}
Expand Down Expand Up @@ -250,7 +253,7 @@ public void addEdge(HugeEdge edge) {

/**
* Add one edge between this vertex and other vertex
*
* <p>
* *** this method is not thread safe, must clone this vertex first before
* multi thread access e.g. `vertex.copy().resetTx();` ***
*/
Expand Down Expand Up @@ -295,7 +298,7 @@ public HugeEdge constructEdge(String label, HugeVertex vertex,
label, this.label(), vertex.label());
// Check sortKeys
List<Id> keys = this.graph().mapPkName2Id(elemKeys.keys());
E.checkArgument(keys.containsAll(edgeLabel.sortKeys()),
E.checkArgument(new HashSet<>(keys).containsAll(edgeLabel.sortKeys()),
"The sort key(s) must be set for the edge " +
"with label: '%s'", edgeLabel.name());

Expand All @@ -304,7 +307,7 @@ public HugeEdge constructEdge(String label, HugeVertex vertex,
Collection<Id> nonNullKeys = CollectionUtils.subtract(
edgeLabel.properties(),
edgeLabel.nullableKeys());
if (!keys.containsAll(nonNullKeys)) {
if (!new HashSet<>(keys).containsAll(nonNullKeys)) {
@SuppressWarnings("unchecked")
Collection<Id> missed = CollectionUtils.subtract(nonNullKeys, keys);
E.checkArgument(false, "All non-null property keys: %s " +
Expand Down Expand Up @@ -419,9 +422,8 @@ public void remove() {

@Watched(prefix = "vertex")
@Override
public <V> VertexProperty<V> property(
VertexProperty.Cardinality cardinality,
String key, V value, Object... objects) {
public <V> VertexProperty<V> property(VertexProperty.Cardinality cardinality,
String key, V value, Object... objects) {
if (objects.length != 0 && objects[0].equals(T.id)) {
throw VertexProperty.Exceptions.userSuppliedIdsNotSupported();
}
Expand All @@ -437,7 +439,7 @@ public <V> VertexProperty<V> property(
* .property(list, "key2", val2)
*
* The cardinality single may be user supplied single, it may also be
* that user doesn't supplied cardinality, when it is latter situation,
* that user doesn't supply cardinality, when it is latter situation,
* we shouldn't check it. Because of this reason, we are forced to
* give up the check of user supplied cardinality single.
* The cardinality not single must be user supplied, so should check it
Expand All @@ -460,10 +462,14 @@ public <V> VertexProperty<V> property(
E.checkArgument(!this.hasProperty(propertyKey.id()),
"Can't update primary key: '%s'", key);
}
if (value == null) {
this.removeProperty(propertyKey.id());
return EmptyVertexProperty.instance();
}
imbajin marked this conversation as resolved.
Show resolved Hide resolved

@SuppressWarnings("unchecked")
VertexProperty<V> prop = (VertexProperty<V>) this.addProperty(
propertyKey, value, !this.fresh());
VertexProperty<V> prop = (VertexProperty<V>) this.addProperty(propertyKey,
value, !this.fresh());
return prop;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.junit.runner.RunWith;

@RunWith(ProcessBasicSuite.class)
@GraphProviderClass(provider = ProcessTestGraphProvider.class,
graph = TestGraph.class)
@GraphProviderClass(provider = ProcessTestGraphProvider.class, graph = TestGraph.class)
public class ProcessStandardTest {
}
Loading