From 627c56f7acd4256bc84473fd61981a83e28c036c Mon Sep 17 00:00:00 2001 From: xtcyclist <7731943+xtcyclist@users.noreply.github.com> Date: Thu, 23 Mar 2023 16:54:50 +0800 Subject: [PATCH 1/7] For comparisions with EMPTY, if the other operand is not EMPTY, return false; otherwise, return null. --- src/common/datatypes/Value.cpp | 10 +++++--- .../test/RelationalExpressionTest.cpp | 24 +++++++++---------- .../expression/BugFixWithngdata.feature | 18 ++++++++++++++ 3 files changed, 37 insertions(+), 15 deletions(-) create mode 100644 tests/tck/features/expression/BugFixWithngdata.feature diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index 79b4a0a48ef..4cb593f5eee 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -1778,7 +1778,7 @@ Value Value::toSet() const { } Value Value::lessThan(const Value& v) const { if (empty() || v.empty()) { - return (v.isNull() || isNull()) ? Value::kNullValue : Value::kEmpty; + return Value::kNullValue; } auto vType = v.type(); auto hasNull = (type_ | vType) & Value::Type::NULLVALUE; @@ -1871,8 +1871,12 @@ Value Value::lessThan(const Value& v) const { } Value Value::equal(const Value& v) const { - if (empty()) { - return v.isNull() ? Value::kNullValue : v.empty(); + if (empty() || v.empty()) { + if (!empty() || !v.empty()) { + return false; + } else { + return Value::kNullValue; + } } auto vType = v.type(); auto hasNull = (type_ | vType) & Value::Type::NULLVALUE; diff --git a/src/common/expression/test/RelationalExpressionTest.cpp b/src/common/expression/test/RelationalExpressionTest.cpp index 571926754f4..7b34903b207 100644 --- a/src/common/expression/test/RelationalExpressionTest.cpp +++ b/src/common/expression/test/RelationalExpressionTest.cpp @@ -100,8 +100,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value()); auto *expr = RelationalExpression::makeEQ(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value(true).type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value(true)) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); } { // empty == null @@ -109,8 +109,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value(NullType::__NULL__)); auto *expr = RelationalExpression::makeEQ(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value(false).type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value(false)) << "check failed: " << expr->toString(); } { // empty != null @@ -118,8 +118,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value(NullType::__NULL__)); auto *expr = RelationalExpression::makeNE(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value(true).type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value(true)) << "check failed: " << expr->toString(); } { // empty != 1 @@ -145,8 +145,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value("1")); auto *expr = RelationalExpression::makeGT(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value::kEmpty.type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value::kEmpty) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); } { // empty < 1 @@ -154,8 +154,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value(1)); auto *expr = RelationalExpression::makeLT(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value::kEmpty.type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value::kEmpty) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); } { // empty >= 1.11 @@ -163,8 +163,8 @@ TEST_F(ExpressionTest, LiteralConstantsRelational) { auto *operand2 = ConstantExpression::make(&pool, Value(1.11)); auto *expr = RelationalExpression::makeGE(&pool, operand1, operand2); auto eval = Expression::eval(expr, gExpCtxt); - EXPECT_EQ(eval.type(), Value::kEmpty.type()) << "type check failed: " << expr->toString(); - EXPECT_EQ(eval, Value::kEmpty) << "check failed: " << expr->toString(); + EXPECT_EQ(eval.type(), Value::kNullValue.type()) << "type check failed: " << expr->toString(); + EXPECT_EQ(eval, Value::kNullValue) << "check failed: " << expr->toString(); } { TEST_EXPR(null != 1, Value::kNullValue); diff --git a/tests/tck/features/expression/BugFixWithngdata.feature b/tests/tck/features/expression/BugFixWithngdata.feature new file mode 100644 index 00000000000..a4f3a1be4ef --- /dev/null +++ b/tests/tck/features/expression/BugFixWithngdata.feature @@ -0,0 +1,18 @@ +# Copyright (c) 2023 vesoft inc. All rights reserved. +# +# This source code is licensed under Apache 2.0 License. +Feature: Bug fixes with ngdata + + Background: + Given a graph with space named "ngdata" + + Scenario: Comparing EMPTY values + When executing query: + """ + MATCH (v0:Label_0)-[e0]->()-[e1*1..1]->(v1) + WHERE (id(v0) == 11) AND (v1.Label_6.Label_6_400_Int == v1.Label_6.Label_6_500_Int) + RETURN count(*) + """ + Then the result should be, in any order: + | count(*) | + | 0 | From c847ed0f36bd3a5e245d7c524ca515bd033c7376 Mon Sep 17 00:00:00 2001 From: xtcyclist <7731943+xtcyclist@users.noreply.github.com> Date: Fri, 24 Mar 2023 20:01:19 +0800 Subject: [PATCH 2/7] patch. --- src/graph/executor/logic/LoopExecutor.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/graph/executor/logic/LoopExecutor.cpp b/src/graph/executor/logic/LoopExecutor.cpp index e45faaa7696..ef04ee86dba 100644 --- a/src/graph/executor/logic/LoopExecutor.cpp +++ b/src/graph/executor/logic/LoopExecutor.cpp @@ -20,6 +20,9 @@ folly::Future LoopExecutor::execute() { QueryExpressionContext ctx(ectx_); auto value = expr->eval(ctx); + if (value.isNull()) { + value = Value(true); + } DCHECK(value.isBool()); finally_ = !(value.isBool() && value.getBool()); return finish(ResultBuilder().value(std::move(value)).iter(Iterator::Kind::kDefault).build()); From f1f1c3b8eccc61b5f71601925a0982f9e5c7d91c Mon Sep 17 00:00:00 2001 From: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Date: Mon, 27 Mar 2023 10:35:15 +0800 Subject: [PATCH 3/7] minor. --- src/common/datatypes/Value.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index 4cb593f5eee..4bbaff007ed 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -1871,12 +1871,8 @@ Value Value::lessThan(const Value& v) const { } Value Value::equal(const Value& v) const { - if (empty() || v.empty()) { - if (!empty() || !v.empty()) { - return false; - } else { - return Value::kNullValue; - } + if UNLIKELY(empty() || v.empty()) { + return !empty() || !v.empty() ? false: Value::kNullValue; } auto vType = v.type(); auto hasNull = (type_ | vType) & Value::Type::NULLVALUE; From 32009f869bc1f1aa4b796a741a94a8b20c1c0ab5 Mon Sep 17 00:00:00 2001 From: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Date: Mon, 27 Mar 2023 10:38:07 +0800 Subject: [PATCH 4/7] minor. --- src/common/datatypes/Value.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index 4bbaff007ed..2d4771d4cce 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -1777,7 +1777,7 @@ Value Value::toSet() const { } } Value Value::lessThan(const Value& v) const { - if (empty() || v.empty()) { + if UNLIKELY(empty() || v.empty()) { return Value::kNullValue; } auto vType = v.type(); From a99654f4cb0e4b8e8b6d514c02c59b01f91fb143 Mon Sep 17 00:00:00 2001 From: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Date: Mon, 27 Mar 2023 11:18:50 +0800 Subject: [PATCH 5/7] minor. --- src/common/datatypes/Value.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index 2d4771d4cce..7a6b5e4fb14 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -1777,7 +1777,7 @@ Value Value::toSet() const { } } Value Value::lessThan(const Value& v) const { - if UNLIKELY(empty() || v.empty()) { + if (UNLIKELY(empty() || v.empty())) { return Value::kNullValue; } auto vType = v.type(); @@ -1871,8 +1871,8 @@ Value Value::lessThan(const Value& v) const { } Value Value::equal(const Value& v) const { - if UNLIKELY(empty() || v.empty()) { - return !empty() || !v.empty() ? false: Value::kNullValue; + if (UNLIKELY(empty() || v.empty())) { + return !empty() || !v.empty() ? false : Value::kNullValue; } auto vType = v.type(); auto hasNull = (type_ | vType) & Value::Type::NULLVALUE; From 3670d500ee4d8bcd150d8047bc76c3cf88e04c23 Mon Sep 17 00:00:00 2001 From: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Date: Mon, 27 Mar 2023 11:48:40 +0800 Subject: [PATCH 6/7] remove some codes. --- src/graph/executor/logic/LoopExecutor.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/graph/executor/logic/LoopExecutor.cpp b/src/graph/executor/logic/LoopExecutor.cpp index ef04ee86dba..45c9d226ade 100644 --- a/src/graph/executor/logic/LoopExecutor.cpp +++ b/src/graph/executor/logic/LoopExecutor.cpp @@ -20,10 +20,6 @@ folly::Future LoopExecutor::execute() { QueryExpressionContext ctx(ectx_); auto value = expr->eval(ctx); - if (value.isNull()) { - value = Value(true); - } - DCHECK(value.isBool()); finally_ = !(value.isBool() && value.getBool()); return finish(ResultBuilder().value(std::move(value)).iter(Iterator::Kind::kDefault).build()); } From d13e36ca8fe1f8bdc9ea57b7686a0b8252067d3f Mon Sep 17 00:00:00 2001 From: Cheng Xuntao <7731943+xtcyclist@users.noreply.github.com> Date: Mon, 27 Mar 2023 12:31:44 +0800 Subject: [PATCH 7/7] revert. --- src/graph/executor/logic/LoopExecutor.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/graph/executor/logic/LoopExecutor.cpp b/src/graph/executor/logic/LoopExecutor.cpp index 45c9d226ade..ca6bb2a11ed 100644 --- a/src/graph/executor/logic/LoopExecutor.cpp +++ b/src/graph/executor/logic/LoopExecutor.cpp @@ -20,6 +20,9 @@ folly::Future LoopExecutor::execute() { QueryExpressionContext ctx(ectx_); auto value = expr->eval(ctx); + if (value.isNull()) { + value = Value(true); + } finally_ = !(value.isBool() && value.getBool()); return finish(ResultBuilder().value(std::move(value)).iter(Iterator::Kind::kDefault).build()); }