Skip to content

Commit

Permalink
fix(interactive): Fix HashCode of Long Literal (#4485)
Browse files Browse the repository at this point in the history
<!--
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
  • Loading branch information
shirly121 authored Feb 13, 2025
1 parent 2d74b02 commit 5bf5f9f
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rex.*;
import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.SqlOperator;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.util.Sarg;
import org.apache.calcite.util.Util;
Expand Down Expand Up @@ -132,4 +133,15 @@ public RexNode makeCast(RelDataType type, RexNode operand) {
}
return super.makeCast(type, operand);
}

@Override
public RexNode makeCall(RelDataType returnType, SqlOperator op, List<RexNode> exprs) {
return new GraphRexCall(returnType, op, exprs);
}

@Override
public RexNode makeCall(SqlOperator op, List<? extends RexNode> exprs) {
RelDataType type = this.deriveReturnType(op, exprs);
return new GraphRexCall(type, op, exprs);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright 2020 Alibaba Group Holding Limited.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.calcite.rex;

import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.SqlOperator;
import org.apache.calcite.sql.SqlSyntax;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;

import java.util.ArrayList;
import java.util.List;

public class GraphRexCall extends RexCall {
public GraphRexCall(RelDataType type, SqlOperator operator, List<? extends RexNode> operands) {
super(type, operator, operands);
}

protected String computeDigest(boolean withType) {
StringBuilder sb = new StringBuilder(this.op.getName());
if (this.operands.size() != 0 || this.op.getSyntax() != SqlSyntax.FUNCTION_ID) {
sb.append("(");
appendOperands0(sb);
sb.append(")");
}

if (withType) {
sb.append(":");
sb.append(this.type.getFullTypeString());
}

return sb.toString();
}

/**
* Re-implements the original Calcite behavior that leads to the type mismatch of BIGINT in conditions like 'a.id = 1L'.
* This type mismatch prevents the query cache from correctly differentiating the hash IDs of conditions such as
* 'a.id = 1L' and 'a.id = 1', even though they require different execution plans.
*
* @param sb The StringBuilder used to construct the query condition.
*/
private void appendOperands0(StringBuilder sb) {
if (operands.isEmpty()) {
return;
}
List<String> operandDigests = new ArrayList<>(operands.size());
for (int i = 0; i < operands.size(); i++) {
RexNode operand = operands.get(i);
if (!(operand instanceof RexLiteral)
|| operand.getType().getSqlTypeName() == SqlTypeName.BIGINT) {
operandDigests.add(operand.toString());
continue;
}
// Type information might be omitted in certain cases to improve readability
// For instance, AND/OR arguments should be BOOLEAN, so
// AND(true, null) is better than AND(true, null:BOOLEAN), and we keep the same info.

// +($0, 2) is better than +($0, 2:BIGINT). Note: if $0 is BIGINT, then 2 is expected to
// be
// of BIGINT type as well.
RexDigestIncludeType includeType = RexDigestIncludeType.OPTIONAL;
if ((isA(SqlKind.AND) || isA(SqlKind.OR))
&& operand.getType().getSqlTypeName() == SqlTypeName.BOOLEAN) {
includeType = RexDigestIncludeType.NO_TYPE;
}
if (SqlKind.SIMPLE_BINARY_OPS.contains(getKind())) {
RexNode otherArg = operands.get(1 - i);
if ((!(otherArg instanceof RexLiteral) || digestSkipsType((RexLiteral) otherArg))
&& SqlTypeUtil.equalSansNullability(
operand.getType(), otherArg.getType())) {
includeType = RexDigestIncludeType.NO_TYPE;
}
}
operandDigests.add(computeDigest((RexLiteral) operand, includeType));
}
int totalLength = (operandDigests.size() - 1) * 2; // commas
for (String s : operandDigests) {
totalLength += s.length();
}
sb.ensureCapacity(sb.length() + totalLength);
for (int i = 0; i < operandDigests.size(); i++) {
String op = operandDigests.get(i);
if (i != 0) {
sb.append(", ");
}
sb.append(op);
}
}

private static boolean digestSkipsType(RexLiteral literal) {
// This seems trivial, however, this method
// workarounds https://github.com/typetools/checker-framework/issues/3631
return literal.digestIncludesType() == RexDigestIncludeType.NO_TYPE;
}

private static String computeDigest(RexLiteral literal, RexDigestIncludeType includeType) {
// This seems trivial, however, this method
// workarounds https://github.com/typetools/checker-framework/issues/3631
return literal.computeDigest(includeType);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,22 @@ public void query_cache_2_test() throws Exception {
// value1 should have been evicted due to max size is 1
Assert.assertTrue(value1 != value3);
}

@Test
public void query_cache_3_test() {
Configs configs = new Configs(ImmutableMap.of("query.cache.size", "10"));
GraphPlanner graphPlanner =
new GraphPlanner(
configs, new LogicalPlanFactory.Gremlin(), new GraphRelOptimizer(configs));
QueryCache cache = new QueryCache(configs);
QueryCache.Key key1 =
cache.createKey(
graphPlanner.instance(
"g.V().hasLabel('person').has('id', 1)", Utils.schemaMeta));
QueryCache.Key key2 =
cache.createKey(
graphPlanner.instance(
"g.V().hasLabel('person').has('id', 1L)", Utils.schemaMeta));
Assert.assertNotEquals(key1.hashCode(), key2.hashCode());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ public void ldbc2_test() {
+ " imageFile=[message.imageFile],"
+ " postOrCommentCreationDate=[message.creationDate], isAppend=[false])\n"
+ " GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[POST, COMMENT]}],"
+ " alias=[message], fusedFilter=[[<(_.creationDate, 20130301000000000)]],"
+ " opt=[START], physicalOpt=[ITSELF])\n"
+ " alias=[message], fusedFilter=[[<(_.creationDate,"
+ " 20130301000000000:BIGINT)]], opt=[START], physicalOpt=[ITSELF])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[HASCREATOR]}],"
+ " alias=[_], startAlias=[friend], opt=[IN], physicalOpt=[VERTEX])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false, tables=[KNOWS]}],"
Expand Down Expand Up @@ -459,7 +459,7 @@ public void ldbc6_test() {
+ " alias=[_], start_alias=[person])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false,"
+ " tables=[PERSON]}], alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id,"
+ " 2199023382370)])",
+ " 2199023382370:BIGINT)])",
after.explain().trim());
}

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

Expand Down Expand Up @@ -616,8 +617,8 @@ public void ldbc9_test() {
+ " commentOrPostCreationDate=[message.creationDate], isAppend=[false])\n"
+ " LogicalFilter(condition=[<>(friend, person)])\n"
+ " GraphPhysicalGetV(tableConfig=[{isAll=false, tables=[POST, COMMENT]}],"
+ " alias=[message], fusedFilter=[[<(_.creationDate, 20130301000000000)]],"
+ " opt=[START], physicalOpt=[ITSELF])\n"
+ " alias=[message], fusedFilter=[[<(_.creationDate,"
+ " 20130301000000000:BIGINT)]], opt=[START], physicalOpt=[ITSELF])\n"
+ " GraphPhysicalExpand(tableConfig=[{isAll=false,"
+ " tables=[HASCREATOR]}], alias=[_], startAlias=[friend], opt=[IN],"
+ " physicalOpt=[VERTEX])\n"
Expand All @@ -630,7 +631,7 @@ public void ldbc9_test() {
+ " alias=[_], start_alias=[person])\n"
+ " GraphLogicalSource(tableConfig=[{isAll=false,"
+ " tables=[PERSON]}], alias=[person], opt=[VERTEX], uniqueKeyFilters=[=(_.id,"
+ " 2199023382370)])",
+ " 2199023382370:BIGINT)])",
after.explain().trim());
}

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

0 comments on commit 5bf5f9f

Please sign in to comment.