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 @@ -230,7 +230,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 @@ -47,12 +47,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 @@ -202,6 +204,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 @@ -281,7 +288,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 @@ -297,7 +304,6 @@ public <V> Iterator<Property<V>> properties(String... keys) {
// Not found
continue;
}
assert prop instanceof Property;
props.add((Property<V>) prop);
}
}
Expand All @@ -322,8 +328,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 @@ -364,6 +369,7 @@ public void vertices(HugeVertex owner, HugeVertex other) {
if (ownerLabel.equals(this.label.sourceLabel())) {
this.vertices(true, owner, other);
} else {
// TODO: why when compare the label but ignore the result?
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe missing assert

ownerLabel.equals(this.label.targetLabel());
this.vertices(false, owner, other);
}
Expand Down Expand Up @@ -517,8 +523,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 @@ -33,14 +33,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 @@ -49,12 +54,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 @@ -63,8 +62,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 @@ -427,11 +426,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) {
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

if (T.label.equals(key)) {
throw Element.Exceptions.labelCanNotBeNull();
}
// 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 @@ -51,8 +51,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