Skip to content

Commit

Permalink
fix(interactive): Fix unexpected alias when fusion Expand + GetV (#…
Browse files Browse the repository at this point in the history
…3676)

<!--
Thanks for your contribution! please review
https://github.com/alibaba/GraphScope/blob/main/CONTRIBUTING.md before
opening an issue.
-->

## What do these changes do?

<!-- Please give a short brief about these changes. -->

As titled.

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this
change? -->

Fixes #3671
  • Loading branch information
BingqingLyu authored Mar 29, 2024
1 parent ff30ce5 commit 79d62d4
Show file tree
Hide file tree
Showing 10 changed files with 89 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,13 @@
// This rule try to fuse GraphLogicalExpand and GraphLogicalGetV if GraphLogicalExpand has no alias
// (that it won't be visited individually later):
// 1. if GraphLogicalGetV has no filters, then:
// GraphLogicalExpand + GraphLogicalGetV -> GraphPhysicalExpand(ExpandV)
// GraphLogicalExpand + GraphLogicalGetV -> GraphPhysicalExpand(ExpandV),
// where GraphPhysicalExpand carries the alias of GraphLogicalGetV
// 2. if GraphLogicalGetV has filters, then:
// GraphLogicalExpand + GraphLogicalGetV -> GraphPhysicalExpand(ExpandV) +
// GraphPhysicalGetV(VertexFilter)
// GraphPhysicalGetV(VertexFilter),
// where GraphPhysicalExpand carries the Default alias, and GraphPhysicalGetV carries the alias of
// GraphLogicalGetV
public abstract class ExpandGetVFusionRule<C extends RelRule.Config> extends RelRule<C>
implements TransformationRule {

Expand All @@ -54,18 +57,27 @@ protected RelNode transform(GraphLogicalGetV getV, GraphLogicalExpand expand, Re
&& getV.getOpt().equals(GraphOpt.GetV.START)
|| expand.getOpt().equals(GraphOpt.Expand.BOTH)
&& getV.getOpt().equals(GraphOpt.GetV.OTHER)) {
GraphPhysicalExpand physicalExpand =
GraphPhysicalExpand.create(
expand.getCluster(),
expand.getHints(),
input,
expand,
getV,
GraphOpt.PhysicalExpandOpt.VERTEX,
getV.getAliasName());
if (ObjectUtils.isEmpty(getV.getFilters())) {
GraphPhysicalExpand physicalExpand =
GraphPhysicalExpand.create(
expand.getCluster(),
expand.getHints(),
input,
expand,
getV,
GraphOpt.PhysicalExpandOpt.VERTEX,
getV.getAliasName());
return physicalExpand;
} else {
GraphPhysicalExpand physicalExpand =
GraphPhysicalExpand.create(
expand.getCluster(),
expand.getHints(),
input,
expand,
getV,
GraphOpt.PhysicalExpandOpt.VERTEX,
AliasInference.DEFAULT_NAME);
// If with filters, then create a GraphPhysicalGetV to do the filtering.
// We set alias of getV to null to avoid alias conflict (with expand's alias)
GraphPhysicalGetV physicalGetV =
Expand All @@ -74,7 +86,7 @@ protected RelNode transform(GraphLogicalGetV getV, GraphLogicalExpand expand, Re
getV.getHints(),
physicalExpand,
getV,
null,
getV.getAliasName(),
GraphOpt.PhysicalGetVOpt.ITSELF);
return physicalGetV;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,31 @@ public static GraphPhysicalExpand create(
GraphLogicalExpand fusedExpand,
GraphLogicalGetV fusedGetV,
GraphOpt.PhysicalExpandOpt physicalOpt,
String alias) {
String aliasName) {
GraphLogicalGetV newGetV = null;
if (fusedGetV != null) {
// if fused to output vertices, build a new getV if a new aliasName is given, to make
// sure the derived row type is correct (which is derived by getV)
if (fusedGetV.getAliasName().equals(aliasName)) {
newGetV = fusedGetV;
} else {
newGetV =
GraphLogicalGetV.create(
(GraphOptCluster) fusedGetV.getCluster(),
fusedGetV.getHints(),
input,
fusedGetV.getOpt(),
fusedGetV.getTableConfig(),
aliasName,
fusedGetV.getStartAlias());
if (ObjectUtils.isNotEmpty(fusedGetV.getFilters())) {
// should not have filters, as it would built as a new PhysicalGetV
newGetV.setFilters(fusedGetV.getFilters());
}
}
}
return new GraphPhysicalExpand(
cluster, hints, input, fusedExpand, fusedGetV, physicalOpt, alias);
cluster, hints, input, fusedExpand, newGetV, physicalOpt, aliasName);
}

public GraphOpt.PhysicalExpandOpt getPhysicalOpt() {
Expand Down Expand Up @@ -104,7 +126,7 @@ public int getAliasId() {
public RelDataType deriveRowType() {
switch (physicalOpt) {
case EDGE:
return fusedExpand.deriveRowType();
return fusedExpand.getRowType();
case DEGREE:
{
RelDataTypeFactory typeFactory = getCluster().getTypeFactory();
Expand All @@ -117,7 +139,7 @@ public RelDataType deriveRowType() {
}
case VERTEX:
default:
return fusedGetV.deriveRowType();
return fusedGetV.getRowType();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.calcite.rel.RelWriter;
import org.apache.calcite.rel.SingleRel;
import org.apache.calcite.rel.hint.RelHint;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.commons.lang3.ObjectUtils;

import java.util.List;
Expand Down Expand Up @@ -102,6 +103,11 @@ public List<RelNode> getInputs() {
return this.input == null ? ImmutableList.of() : ImmutableList.of(this.input);
}

@Override
public RelDataType deriveRowType() {
return fusedGetV.getRowType();
}

@Override
public RelWriter explainTerms(RelWriter pw) {
return pw.itemIf("input", input, !Objects.isNull(input))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,10 @@ public void expand_getv_fusion_4_test() {
planner.setRoot(before);
RelNode after = planner.findBestExp();
Assert.assertEquals(
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[_],"
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[a],"
+ " fusedFilter=[[=(_.age, 10)]], opt=[END], physicalOpt=[ITSELF])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[knows]}],"
+ " alias=[a], opt=[OUT], physicalOpt=[VERTEX])\n"
+ " alias=[_], opt=[OUT], physicalOpt=[VERTEX])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[person]}],"
+ " alias=[_], opt=[VERTEX])",
after.explain().trim());
Expand Down Expand Up @@ -282,10 +282,10 @@ public void expand_getv_fusion_5_test() {
planner.setRoot(before);
RelNode after = planner.findBestExp();
Assert.assertEquals(
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[_],"
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[a],"
+ " fusedFilter=[[=(_.age, 10)]], opt=[END], physicalOpt=[ITSELF])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[knows]}],"
+ " alias=[a], fusedFilter=[[=(_.weight, 5E-1)]], opt=[OUT],"
+ " alias=[_], fusedFilter=[[=(_.weight, 5E-1)]], opt=[OUT],"
+ " physicalOpt=[VERTEX])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[person]}],"
+ " alias=[_], opt=[VERTEX])",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ public void expand_degree_test() throws Exception {
}
}

// g.V().hasLabel("person").outE("knows").inV()
// g.V().hasLabel("person").outE("knows").inV().as("a")
@Test
public void expand_vertex_test() throws Exception {
GraphBuilder builder = Utils.mockGraphBuilder();
Expand All @@ -879,11 +879,12 @@ public void expand_vertex_test() throws Exception {
.getV(
new GetVConfig(
GraphOpt.GetV.END,
new LabelConfig(false).addLabel("person")))
new LabelConfig(false).addLabel("person"),
"a"))
.build();
Assert.assertEquals(
"GraphLogicalGetV(tableConfig=[{isAll=false, tables=[person]}],"
+ " alias=[_], opt=[END])\n"
+ " alias=[a], opt=[END])\n"
+ " GraphLogicalExpand(tableConfig=[{isAll=false, tables=[knows]}],"
+ " alias=[_], opt=[OUT])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[person]}],"
Expand All @@ -894,7 +895,7 @@ public void expand_vertex_test() throws Exception {
planner.setRoot(before);
RelNode after = planner.findBestExp();
Assert.assertEquals(
"GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[knows]}], alias=[_],"
"GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[knows]}], alias=[a],"
+ " opt=[OUT], physicalOpt=[VERTEX])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[person]}],"
+ " alias=[_], opt=[VERTEX])",
Expand All @@ -921,7 +922,7 @@ public void expand_vertex_test() throws Exception {
}
}

// g.V().hasLabel("person").outE("knows").inV().has("age",10), can be fused into
// g.V().hasLabel("person").outE("knows").inV().as("a").has("age",10), can be fused into
// GraphPhysicalExpand + GraphPhysicalGetV
@Test
public void expand_vertex_filter_test() throws Exception {
Expand Down Expand Up @@ -1015,10 +1016,10 @@ public void expand_vertex_with_filters_test() throws Exception {
planner.setRoot(before);
RelNode after = planner.findBestExp();
Assert.assertEquals(
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[_],"
"GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[person]}], alias=[a],"
+ " fusedFilter=[[=(_.age, 10)]], opt=[END], physicalOpt=[ITSELF])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[knows]}],"
+ " alias=[a], fusedFilter=[[=(_.weight, 5E-1)]], opt=[OUT],"
+ " alias=[_], fusedFilter=[[=(_.weight, 5E-1)]], opt=[OUT],"
+ " physicalOpt=[VERTEX])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[person]}],"
+ " alias=[_], opt=[VERTEX])",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@
"id": 0
}],
"sampleRatio": 1.0
},
"alias": 0
}
}
},
"metaData": [{
Expand All @@ -73,7 +72,8 @@
}]
}]
}
}
},
"alias": -1
}]
}, {
"opr": {
Expand Down Expand Up @@ -113,7 +113,8 @@
}]
},
"sampleRatio": 1.0
}
},
"alias": 0
}
},
"metaData": [{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"id": 0
}],
"sampleRatio": 1.0
}
},
"alias": 0
}
},
"metaData": [{
Expand All @@ -72,8 +73,7 @@
}]
}]
}
},
"alias": -1
}
}]
}, {
"opr": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@
}]
},
"sampleRatio": 1.0
},
"alias": 0
}
}
},
"metaData": [{
Expand All @@ -102,7 +101,8 @@
}]
}]
}
}
},
"alias": -1
}]
}, {
"opr": {
Expand Down Expand Up @@ -142,7 +142,8 @@
}]
},
"sampleRatio": 1.0
}
},
"alias": 0
}
},
"metaData": [{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@
"id": 0
}],
"sampleRatio": 1.0
},
"alias": 0
}
}
},
"metaData": [{
Expand All @@ -80,7 +79,8 @@
}]
}]
}
}
},
"alias": -1
}]
}, {
"opr": {
Expand Down Expand Up @@ -127,7 +127,8 @@
}]
},
"sampleRatio": 1.0
}
},
"alias": 0
}
},
"metaData": [{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@
"id": 0
}],
"sampleRatio": 1.0
}
},
"alias": 0
}
},
"metaData": [{
Expand All @@ -79,8 +80,7 @@
}]
}]
}
},
"alias": -1
}
}]
}, {
"opr": {
Expand Down

0 comments on commit 79d62d4

Please sign in to comment.