Skip to content

Commit 5bf5f9f

Browse files
authored
fix(interactive): Fix HashCode of Long Literal (#4485)
<!-- Thanks for your contribution! please review https://github.com/alibaba/GraphScope/blob/main/CONTRIBUTING.md before opening an issue. --> ## What do these changes do? as titled. <!-- Please give a short brief about these changes. --> ## Related issue number <!-- Are there any issues opened that will be resolved by merging this change? --> Fixes #4484
1 parent 2d74b02 commit 5bf5f9f

File tree

4 files changed

+155
-8
lines changed

4 files changed

+155
-8
lines changed

interactive_engine/compiler/src/main/java/com/alibaba/graphscope/common/ir/tools/GraphRexBuilder.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.apache.calcite.rel.type.RelDataTypeFactory;
2525
import org.apache.calcite.rex.*;
2626
import org.apache.calcite.sql.SqlKind;
27+
import org.apache.calcite.sql.SqlOperator;
2728
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
2829
import org.apache.calcite.util.Sarg;
2930
import org.apache.calcite.util.Util;
@@ -132,4 +133,15 @@ public RexNode makeCast(RelDataType type, RexNode operand) {
132133
}
133134
return super.makeCast(type, operand);
134135
}
136+
137+
@Override
138+
public RexNode makeCall(RelDataType returnType, SqlOperator op, List<RexNode> exprs) {
139+
return new GraphRexCall(returnType, op, exprs);
140+
}
141+
142+
@Override
143+
public RexNode makeCall(SqlOperator op, List<? extends RexNode> exprs) {
144+
RelDataType type = this.deriveReturnType(op, exprs);
145+
return new GraphRexCall(type, op, exprs);
146+
}
135147
}
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Copyright 2020 Alibaba Group Holding Limited.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.apache.calcite.rex;
18+
19+
import org.apache.calcite.rel.type.RelDataType;
20+
import org.apache.calcite.sql.SqlKind;
21+
import org.apache.calcite.sql.SqlOperator;
22+
import org.apache.calcite.sql.SqlSyntax;
23+
import org.apache.calcite.sql.type.SqlTypeName;
24+
import org.apache.calcite.sql.type.SqlTypeUtil;
25+
26+
import java.util.ArrayList;
27+
import java.util.List;
28+
29+
public class GraphRexCall extends RexCall {
30+
public GraphRexCall(RelDataType type, SqlOperator operator, List<? extends RexNode> operands) {
31+
super(type, operator, operands);
32+
}
33+
34+
protected String computeDigest(boolean withType) {
35+
StringBuilder sb = new StringBuilder(this.op.getName());
36+
if (this.operands.size() != 0 || this.op.getSyntax() != SqlSyntax.FUNCTION_ID) {
37+
sb.append("(");
38+
appendOperands0(sb);
39+
sb.append(")");
40+
}
41+
42+
if (withType) {
43+
sb.append(":");
44+
sb.append(this.type.getFullTypeString());
45+
}
46+
47+
return sb.toString();
48+
}
49+
50+
/**
51+
* Re-implements the original Calcite behavior that leads to the type mismatch of BIGINT in conditions like 'a.id = 1L'.
52+
* This type mismatch prevents the query cache from correctly differentiating the hash IDs of conditions such as
53+
* 'a.id = 1L' and 'a.id = 1', even though they require different execution plans.
54+
*
55+
* @param sb The StringBuilder used to construct the query condition.
56+
*/
57+
private void appendOperands0(StringBuilder sb) {
58+
if (operands.isEmpty()) {
59+
return;
60+
}
61+
List<String> operandDigests = new ArrayList<>(operands.size());
62+
for (int i = 0; i < operands.size(); i++) {
63+
RexNode operand = operands.get(i);
64+
if (!(operand instanceof RexLiteral)
65+
|| operand.getType().getSqlTypeName() == SqlTypeName.BIGINT) {
66+
operandDigests.add(operand.toString());
67+
continue;
68+
}
69+
// Type information might be omitted in certain cases to improve readability
70+
// For instance, AND/OR arguments should be BOOLEAN, so
71+
// AND(true, null) is better than AND(true, null:BOOLEAN), and we keep the same info.
72+
73+
// +($0, 2) is better than +($0, 2:BIGINT). Note: if $0 is BIGINT, then 2 is expected to
74+
// be
75+
// of BIGINT type as well.
76+
RexDigestIncludeType includeType = RexDigestIncludeType.OPTIONAL;
77+
if ((isA(SqlKind.AND) || isA(SqlKind.OR))
78+
&& operand.getType().getSqlTypeName() == SqlTypeName.BOOLEAN) {
79+
includeType = RexDigestIncludeType.NO_TYPE;
80+
}
81+
if (SqlKind.SIMPLE_BINARY_OPS.contains(getKind())) {
82+
RexNode otherArg = operands.get(1 - i);
83+
if ((!(otherArg instanceof RexLiteral) || digestSkipsType((RexLiteral) otherArg))
84+
&& SqlTypeUtil.equalSansNullability(
85+
operand.getType(), otherArg.getType())) {
86+
includeType = RexDigestIncludeType.NO_TYPE;
87+
}
88+
}
89+
operandDigests.add(computeDigest((RexLiteral) operand, includeType));
90+
}
91+
int totalLength = (operandDigests.size() - 1) * 2; // commas
92+
for (String s : operandDigests) {
93+
totalLength += s.length();
94+
}
95+
sb.ensureCapacity(sb.length() + totalLength);
96+
for (int i = 0; i < operandDigests.size(); i++) {
97+
String op = operandDigests.get(i);
98+
if (i != 0) {
99+
sb.append(", ");
100+
}
101+
sb.append(op);
102+
}
103+
}
104+
105+
private static boolean digestSkipsType(RexLiteral literal) {
106+
// This seems trivial, however, this method
107+
// workarounds https://github.com/typetools/checker-framework/issues/3631
108+
return literal.digestIncludesType() == RexDigestIncludeType.NO_TYPE;
109+
}
110+
111+
private static String computeDigest(RexLiteral literal, RexDigestIncludeType includeType) {
112+
// This seems trivial, however, this method
113+
// workarounds https://github.com/typetools/checker-framework/issues/3631
114+
return literal.computeDigest(includeType);
115+
}
116+
}

interactive_engine/compiler/src/test/java/com/alibaba/graphscope/common/ir/QueryCacheTest.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,22 @@ public void query_cache_2_test() throws Exception {
7676
// value1 should have been evicted due to max size is 1
7777
Assert.assertTrue(value1 != value3);
7878
}
79+
80+
@Test
81+
public void query_cache_3_test() {
82+
Configs configs = new Configs(ImmutableMap.of("query.cache.size", "10"));
83+
GraphPlanner graphPlanner =
84+
new GraphPlanner(
85+
configs, new LogicalPlanFactory.Gremlin(), new GraphRelOptimizer(configs));
86+
QueryCache cache = new QueryCache(configs);
87+
QueryCache.Key key1 =
88+
cache.createKey(
89+
graphPlanner.instance(
90+
"g.V().hasLabel('person').has('id', 1)", Utils.schemaMeta));
91+
QueryCache.Key key2 =
92+
cache.createKey(
93+
graphPlanner.instance(
94+
"g.V().hasLabel('person').has('id', 1L)", Utils.schemaMeta));
95+
Assert.assertNotEquals(key1.hashCode(), key2.hashCode());
96+
}
7997
}

interactive_engine/compiler/src/test/java/com/alibaba/graphscope/common/ir/planner/cbo/LdbcTest.java

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,8 @@ public void ldbc2_test() {
183183
+ " imageFile=[message.imageFile],"
184184
+ " postOrCommentCreationDate=[message.creationDate], isAppend=[false])\n"
185185
+ " GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[POST, COMMENT]}],"
186-
+ " alias=[message], fusedFilter=[[<(_.creationDate, 20130301000000000)]],"
187-
+ " opt=[START], physicalOpt=[ITSELF])\n"
186+
+ " alias=[message], fusedFilter=[[<(_.creationDate,"
187+
+ " 20130301000000000:BIGINT)]], opt=[START], physicalOpt=[ITSELF])\n"
188188
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[HASCREATOR]}],"
189189
+ " alias=[_], startAlias=[friend], opt=[IN], physicalOpt=[VERTEX])\n"
190190
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[KNOWS]}],"
@@ -459,7 +459,7 @@ public void ldbc6_test() {
459459
+ " alias=[_], start_alias=[person])\n"
460460
+ " GraphLogicalSource(tableConfig=[{isAll=false,"
461461
+ " tables=[PERSON]}], alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id,"
462-
+ " 2199023382370)])",
462+
+ " 2199023382370:BIGINT)])",
463463
after.explain().trim());
464464
}
465465

@@ -581,7 +581,8 @@ public void ldbc8_test() {
581581
+ " tables=[HASCREATOR]}], alias=[message], startAlias=[person], opt=[IN],"
582582
+ " physicalOpt=[VERTEX])\n"
583583
+ " GraphLogicalSource(tableConfig=[{isAll=false, tables=[PERSON]}],"
584-
+ " alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id, 2199023382370)])",
584+
+ " alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id,"
585+
+ " 2199023382370:BIGINT)])",
585586
after.explain().trim());
586587
}
587588

@@ -616,8 +617,8 @@ public void ldbc9_test() {
616617
+ " commentOrPostCreationDate=[message.creationDate], isAppend=[false])\n"
617618
+ " LogicalFilter(condition=[<>(friend, person)])\n"
618619
+ " GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[POST, COMMENT]}],"
619-
+ " alias=[message], fusedFilter=[[<(_.creationDate, 20130301000000000)]],"
620-
+ " opt=[START], physicalOpt=[ITSELF])\n"
620+
+ " alias=[message], fusedFilter=[[<(_.creationDate,"
621+
+ " 20130301000000000:BIGINT)]], opt=[START], physicalOpt=[ITSELF])\n"
621622
+ " GraphPhysicalExpand(tableConfig=[{isAll=false,"
622623
+ " tables=[HASCREATOR]}], alias=[_], startAlias=[friend], opt=[IN],"
623624
+ " physicalOpt=[VERTEX])\n"
@@ -630,7 +631,7 @@ public void ldbc9_test() {
630631
+ " alias=[_], start_alias=[person])\n"
631632
+ " GraphLogicalSource(tableConfig=[{isAll=false,"
632633
+ " tables=[PERSON]}], alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id,"
633-
+ " 2199023382370)])",
634+
+ " 2199023382370:BIGINT)])",
634635
after.explain().trim());
635636
}
636637

@@ -854,7 +855,7 @@ public void ldbc12_test() {
854855
+ " opt=[BOTH], physicalOpt=[VERTEX])\n"
855856
+ " GraphLogicalSource(tableConfig=[{isAll=false,"
856857
+ " tables=[PERSON]}], alias=[PATTERN_VERTEX$0], opt=[VERTEX],"
857-
+ " uniqueKeyFilters=[=(_.id, 2199023382370)])",
858+
+ " uniqueKeyFilters=[=(_.id, 2199023382370:BIGINT)])",
858859
after.explain().trim());
859860
}
860861
}

0 commit comments

Comments
 (0)