From 11d6ae6ee9e56ee06a43dab8fd9db884ae36b1e7 Mon Sep 17 00:00:00 2001 From: Xiaoli Zhou Date: Thu, 14 Sep 2023 19:34:09 +0800 Subject: [PATCH] fix(interactive): Fix bugs in Label Name to Id Conversion (#3221) ## What do these changes do? When converting a label name into an ID, it is necessary to first infer whether it is a vertex label or an edge label based on the operator preceding the elementMap. There will be issues when the preceding operator falls into one of the following categories: 1. When the preceding operator is an `ExpandFusionStep` after optimization (fusion of outE + filter + inV); 2. When the preceding operator is an operator like `limit` or `where` that does not change the input type. This PR addresses the two issues mentioned above. ## Related issue number fix #3219 Fixes Co-authored-by: Longbin Lai --- .../suite/standard/IrGremlinQueryTest.java | 125 ++++++++++++++++++ .../gremlin/result/GremlinResultAnalyzer.java | 27 ++-- .../gremlin/result/LabelParser.java | 28 +++- 3 files changed, 164 insertions(+), 16 deletions(-) diff --git a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/integration/suite/standard/IrGremlinQueryTest.java b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/integration/suite/standard/IrGremlinQueryTest.java index 3f7920f41c0f..8ca155d066ae 100644 --- a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/integration/suite/standard/IrGremlinQueryTest.java +++ b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/integration/suite/standard/IrGremlinQueryTest.java @@ -70,6 +70,14 @@ public abstract class IrGremlinQueryTest extends AbstractGremlinProcessTest { public abstract Traversal> get_g_V_a_out_b_select_a_b_by_label_id(); + public abstract Traversal> get_g_V_outE_hasLabel_inV_elementMap(); + + public abstract Traversal> get_g_V_limit_elementMap(); + + public abstract Traversal get_g_V_limit_label(); + + public abstract Traversal> get_g_V_a_out_b_select_by_elementMap(); + public abstract Traversal> get_g_V_group_by_by_sum(); public abstract Traversal> get_g_V_group_by_by_max(); @@ -302,6 +310,97 @@ public void g_V_a_out_b_select_a_b_by_label_id() { Assert.assertEquals("{a=person, b=lop}", traversal.next().toString()); } + @LoadGraphWith(LoadGraphWith.GraphData.MODERN) + @Test + public void g_V_outE_hasLabel_inV_elementMap() { + final Traversal> traversal = + get_g_V_outE_hasLabel_inV_elementMap(); + printTraversalForm(traversal); + int counter = 0; + while (traversal.hasNext()) { + Map values = traversal.next(); + Assert.assertTrue(values.containsKey(T.id)); + String name = (String) values.get("name"); + if (name.equals("vadas")) { + Assert.assertEquals(27, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else if (name.equals("josh")) { + Assert.assertEquals(32, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else { + throw new IllegalStateException("It is not possible to reach here: " + values); + } + ++counter; + } + Assert.assertEquals(2, counter); + } + + @LoadGraphWith(LoadGraphWith.GraphData.MODERN) + @Test + public void g_V_limit_elementMap() { + final Traversal> traversal = get_g_V_limit_elementMap(); + printTraversalForm(traversal); + Map values = traversal.next(); + Assert.assertTrue(values.containsKey(T.id)); + String name = (String) values.get("name"); + if (name.equals("marko")) { + Assert.assertEquals(29, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else if (name.equals("josh")) { + Assert.assertEquals(32, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else if (name.equals("peter")) { + Assert.assertEquals(35, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else if (name.equals("vadas")) { + Assert.assertEquals(27, values.get("age")); + Assert.assertEquals("person", values.get(T.label)); + } else if (name.equals("lop")) { + Assert.assertEquals("java", values.get("lang")); + Assert.assertEquals("software", values.get(T.label)); + } else { + if (!name.equals("ripple")) { + throw new IllegalStateException("It is not possible to reach here: " + values); + } + + Assert.assertEquals("java", values.get("lang")); + Assert.assertEquals("software", values.get(T.label)); + } + } + + @LoadGraphWith(LoadGraphWith.GraphData.MODERN) + @Test + public void g_V_limit_label() { + final Traversal traversal = get_g_V_limit_label(); + printTraversalForm(traversal); + String value = traversal.next(); + Assert.assertTrue(value.equals("person") || value.equals("software")); + } + + @LoadGraphWith(LoadGraphWith.GraphData.MODERN) + @Test + public void g_V_a_out_b_select_by_elementMap() { + final Traversal> traversal = + get_g_V_a_out_b_select_by_elementMap(); + printTraversalForm(traversal); + + Map values = traversal.next(); + + Map value1 = (Map) values.get("a"); + Assert.assertTrue(value1.containsKey(T.id)); + Assert.assertEquals("marko", value1.get("name")); + Assert.assertEquals(29, value1.get("age")); + Assert.assertEquals("person", value1.get(T.label)); + + Map value2 = (Map) values.get("b"); + Assert.assertTrue(value2.containsKey(T.id)); + Assert.assertEquals("josh", value2.get("name")); + Assert.assertEquals(32, value2.get("age")); + Assert.assertEquals("person", value2.get(T.label)); + + Assert.assertTrue(!traversal.hasNext()); + } + @LoadGraphWith(LoadGraphWith.GraphData.MODERN) @Test public void g_V_group_by_by_sum() { @@ -640,6 +739,32 @@ public Traversal> get_g_V_a_out_b_select_a_b_by_labe .by("name"); } + @Override + public Traversal> get_g_V_outE_hasLabel_inV_elementMap() { + return g.V().outE().hasLabel("knows").inV().elementMap(); + } + + @Override + public Traversal> get_g_V_limit_elementMap() { + return g.V().limit(1).elementMap(); + } + + @Override + public Traversal get_g_V_limit_label() { + return g.V().limit(1).label(); + } + + @Override + public Traversal> get_g_V_a_out_b_select_by_elementMap() { + return g.V().has("name", "marko") + .as("a") + .out() + .has("name", "josh") + .as("b") + .select("a", "b") + .by(elementMap()); + } + @Override public Traversal> get_g_V_group_by_by_sum() { return g.V().group().by().by(values("age").sum()); diff --git a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/GremlinResultAnalyzer.java b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/GremlinResultAnalyzer.java index 70343405aad5..d1850790eefd 100644 --- a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/GremlinResultAnalyzer.java +++ b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/GremlinResultAnalyzer.java @@ -71,17 +71,7 @@ public static GremlinResultParser analyze(Traversal traversal) { parserType = UnionResultParser.create(step); } else if (Utils.equalClass(step, SubgraphStep.class)) { parserType = GremlinResultParserFactory.SUBGRAPH; - } else if (Utils.equalClass(step, HasStep.class) - || Utils.equalClass(step, DedupGlobalStep.class) - || Utils.equalClass(step, RangeGlobalStep.class) - || Utils.equalClass(step, OrderGlobalStep.class) - || Utils.equalClass(step, IsStep.class) - || Utils.equalClass(step, WherePredicateStep.class) - || Utils.equalClass(step, TraversalFilterStep.class) - || Utils.equalClass(step, WhereTraversalStep.class) - || Utils.equalClass(step, NotStep.class) - || Utils.equalClass(step, CoinStep.class) - || Utils.equalClass(step, SampleGlobalStep.class)) { + } else if (isSameInAndOutputType(step)) { // do nothing; } else { throw new UnsupportedStepException(step.getClass(), "unimplemented yet"); @@ -89,4 +79,19 @@ public static GremlinResultParser analyze(Traversal traversal) { } return parserType; } + + // the step has the same input and output data type + public static boolean isSameInAndOutputType(Step step) { + return Utils.equalClass(step, HasStep.class) + || Utils.equalClass(step, DedupGlobalStep.class) + || Utils.equalClass(step, RangeGlobalStep.class) + || Utils.equalClass(step, OrderGlobalStep.class) + || Utils.equalClass(step, IsStep.class) + || Utils.equalClass(step, WherePredicateStep.class) + || Utils.equalClass(step, TraversalFilterStep.class) + || Utils.equalClass(step, WhereTraversalStep.class) + || Utils.equalClass(step, NotStep.class) + || Utils.equalClass(step, CoinStep.class) + || Utils.equalClass(step, SampleGlobalStep.class); + } } diff --git a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/LabelParser.java b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/LabelParser.java index cf29e801f066..36d2b5079e78 100644 --- a/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/LabelParser.java +++ b/interactive_engine/compiler/src/main/java/com/alibaba/graphscope/gremlin/result/LabelParser.java @@ -18,6 +18,7 @@ import com.alibaba.graphscope.common.jna.type.FfiKeyType; import com.alibaba.graphscope.gaia.proto.Common; +import com.alibaba.graphscope.gremlin.plugin.step.ExpandFusionStep; import com.alibaba.graphscope.gremlin.transform.TraversalParentTransformFactory; import com.google.common.collect.Lists; @@ -130,15 +131,32 @@ private LabelType getLabelType(String tag, Object traversalOrStep, Step parent) ? parent : (Step) traversalOrStep; } - do { - tagStep = tagStep.getPreviousStep(); - } while (tagStep != EmptyStep.instance() - && !ObjectUtils.isEmpty(tag) - && !tagStep.getLabels().contains(tag)); + tagStep = tagStep.getPreviousStep(); + while (tagStep != EmptyStep.instance()) { + if (ObjectUtils.isEmpty(tag) || tagStep.getLabels().contains(tag)) { + while (tagStep != EmptyStep.instance() + && GremlinResultAnalyzer.isSameInAndOutputType(tagStep)) { + tagStep = tagStep.getPreviousStep(); + } + break; + } else { + tagStep = tagStep.getPreviousStep(); + } + } if (tagStep instanceof GraphStep) { return ((GraphStep) tagStep).returnsVertex() ? LabelType.VERTEX_LABEL : LabelType.EDGE_LABEL; + } else if (tagStep instanceof ExpandFusionStep) { + switch (((ExpandFusionStep) tagStep).getExpandOpt()) { + case Vertex: + return LabelType.VERTEX_LABEL; + case Edge: + return LabelType.EDGE_LABEL; + case Degree: + default: + return LabelType.NONE; + } } else if (tagStep instanceof VertexStep) { return ((VertexStep) tagStep).returnsVertex() ? LabelType.VERTEX_LABEL