From 5e9d15e7183509f5515cfd33287f60c39848aebb Mon Sep 17 00:00:00 2001 From: cpw <13495049+CPWstatic@users.noreply.github.com> Date: Mon, 2 Aug 2021 14:48:20 +0800 Subject: [PATCH] v2.5.0 cherry-pick some bug fix from master. (#597) * Modify the return value of partId function (#591) * fix and simplify SubscriptRangeExpression's eval (#593) * Fix toInteger() (#595) * Fix toInteger/toFloat parsing string * Check toInteger() overflow when parsing string Co-authored-by: laura-ding <48548375+laura-ding@users.noreply.github.com> Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com> Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com> --- src/common/clients/meta/MetaClient.cpp | 2 +- src/common/clients/meta/MetaClient.h | 2 +- .../clients/storage/InternalStorageClient.cpp | 7 +- src/common/datatypes/Value.cpp | 19 +- src/common/datatypes/test/ValueTest.cpp | 166 ++++++++++++------ src/common/expression/SubscriptExpression.cpp | 75 +++----- .../test/SubscriptExpressionTest.cpp | 9 + 7 files changed, 172 insertions(+), 108 deletions(-) diff --git a/src/common/clients/meta/MetaClient.cpp b/src/common/clients/meta/MetaClient.cpp index 4c47f6b5c..d03cc0f5e 100644 --- a/src/common/clients/meta/MetaClient.cpp +++ b/src/common/clients/meta/MetaClient.cpp @@ -1030,7 +1030,7 @@ void MetaClient::loadRemoteListeners() { /// ================================== public methods ================================= -StatusOr MetaClient::partId(int32_t numParts, const VertexID id) const { +PartitionID MetaClient::partId(int32_t numParts, const VertexID id) const { // If the length of the id is 8, we will treat it as int64_t to be compatible // with the version 1.0 uint64_t vid = 0; diff --git a/src/common/clients/meta/MetaClient.h b/src/common/clients/meta/MetaClient.h index 2b8373c29..e52e670b1 100644 --- a/src/common/clients/meta/MetaClient.h +++ b/src/common/clients/meta/MetaClient.h @@ -511,7 +511,7 @@ class MetaClient { StatusOr partsNum(GraphSpaceID spaceId) const; - StatusOr partId(int32_t numParts, VertexID id) const; + PartitionID partId(int32_t numParts, VertexID id) const; StatusOr> getTagSchemaFromCache(GraphSpaceID spaceId, TagID tagID, SchemaVer ver = -1); diff --git a/src/common/clients/storage/InternalStorageClient.cpp b/src/common/clients/storage/InternalStorageClient.cpp index 493599004..bf5f2aae2 100644 --- a/src/common/clients/storage/InternalStorageClient.cpp +++ b/src/common/clients/storage/InternalStorageClient.cpp @@ -99,13 +99,10 @@ folly::SemiFuture InternalStorageClient::getValue(size_t vIdLen, folly::StringPiece key, folly::EventBase* evb) { auto srcVid = key.subpiece(sizeof(PartitionID), vIdLen); - auto stPartId = metaClient_->partId(spaceId, srcVid.str()); - if (!stPartId.ok()) { - return nebula::cpp2::ErrorCode::E_SPACE_NOT_FOUND; - } + auto partId = metaClient_->partId(spaceId, srcVid.str()); auto c = folly::makePromiseContract(); - getValueImpl(spaceId, stPartId.value(), key.str(), std::move(c.first), evb); + getValueImpl(spaceId, partId, key.str(), std::move(c.first), evb); return std::move(c.second); } diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index f8fb10d37..dffc3743a 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -1595,7 +1595,7 @@ Value Value::toFloat() const { } case Value::Type::STRING: { const auto& str = getStr(); - char *pEnd; + char* pEnd; double val = strtod(str.c_str(), &pEnd); if (*pEnd != '\0') { return Value::kNullValue; @@ -1629,12 +1629,23 @@ Value Value::toInt() const { } case Value::Type::STRING: { const auto& str = getStr(); - char *pEnd; - double val = strtod(str.c_str(), &pEnd); + errno = 0; + char* pEnd; + auto it = std::find(str.begin(), str.end(), '.'); + int64_t val = + (it == str.end()) + ? int64_t(strtoll(str.c_str(), &pEnd, 10)) // string representing int + : int64_t(strtod(str.c_str(), &pEnd)); // string representing double if (*pEnd != '\0') { return Value::kNullValue; } - return static_cast(val); + // Check overflow + if ((val == std::numeric_limits::max() || + val == std::numeric_limits::min()) && + errno == ERANGE) { + return kNullOverflow; + } + return val; } default: { return Value::kNullBadType; diff --git a/src/common/datatypes/test/ValueTest.cpp b/src/common/datatypes/test/ValueTest.cpp index 4c5135174..ca2385e88 100644 --- a/src/common/datatypes/test/ValueTest.cpp +++ b/src/common/datatypes/test/ValueTest.cpp @@ -638,76 +638,142 @@ TEST(Value, TypeCast) { EXPECT_EQ(Value::Type::NULLVALUE, vb8.type()); } { - auto vf1 = vInt.toFloat(); - EXPECT_EQ(Value::Type::FLOAT, vf1.type()); - EXPECT_EQ(vf1.getFloat(), 1.0); + auto vf = vInt.toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), 1.0); - auto vf2 = vFloat.toFloat(); - EXPECT_EQ(Value::Type::FLOAT, vf2.type()); - EXPECT_EQ(vf2.getFloat(), 3.14); + vf = vFloat.toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), 3.14); - auto vf3 = vStr1.toFloat(); - EXPECT_EQ(Value::Type::NULLVALUE, vf3.type()); + vf = vStr1.toFloat(); + EXPECT_EQ(Value::Type::NULLVALUE, vf.type()); - auto vf4 = vStr2.toFloat(); - EXPECT_EQ(Value::Type::FLOAT, vf4.type()); - EXPECT_EQ(vf4.getFloat(), 3.14); + vf = vStr2.toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), 3.14); - auto vf5 = vBool1.toFloat(); - EXPECT_EQ(Value::Type::NULLVALUE, vf5.type()); + vf = vBool1.toFloat(); + EXPECT_EQ(Value::Type::NULLVALUE, vf.type()); - auto vf6 = vBool2.toFloat(); - EXPECT_EQ(Value::Type::NULLVALUE, vf6.type()); + vf = vBool2.toFloat(); + EXPECT_EQ(Value::Type::NULLVALUE, vf.type()); - auto vf7 = vDate.toFloat(); - EXPECT_EQ(Value::Type::NULLVALUE, vf7.type()); + vf = vDate.toFloat(); + EXPECT_EQ(Value::Type::NULLVALUE, vf.type()); - auto vf8 = vNull.toFloat(); - EXPECT_EQ(Value::Type::NULLVALUE, vf8.type()); + vf = vNull.toFloat(); + EXPECT_EQ(Value::Type::NULLVALUE, vf.type()); - auto vf9 = vIntMin.toFloat(); - EXPECT_EQ(Value::Type::FLOAT, vf9.type()); - EXPECT_EQ(vf9.getFloat(), std::numeric_limits::min()); + vf = vIntMin.toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), std::numeric_limits::min()); - auto vf10 = vIntMax.toFloat(); - EXPECT_EQ(Value::Type::FLOAT, vf10.type()); - EXPECT_EQ(vf10.getFloat(), std::numeric_limits::max()); + vf = vIntMax.toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), static_cast(std::numeric_limits::max())); + + // String of int + vf = Value("-9223372036854775807").toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), static_cast(std::numeric_limits::min() + 1)); + + vf = Value("-9223372036854775808") + .toFloat(); // will be converted to -9223372036854776000.0 + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), static_cast(std::numeric_limits::min())); + + vf = Value("9223372036854775807").toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), static_cast(std::numeric_limits::max())); + + // String of double + vf = Value("123.").toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), 123.0); + + vf = Value(std::to_string(std::numeric_limits::max())).toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), std::numeric_limits::max()); + + vf = Value(std::to_string(std::numeric_limits::max())).toFloat(); + EXPECT_EQ(Value::Type::FLOAT, vf.type()); + EXPECT_EQ(vf.getFloat(), std::numeric_limits::max()); + + // Invlaid string + vf = Value("12abc").toFloat(); + EXPECT_EQ(Value::kNullValue, vf); + + vf = Value("1.2abc").toFloat(); + EXPECT_EQ(Value::kNullValue, vf); } { - auto vi1 = vInt.toInt(); - EXPECT_EQ(Value::Type::INT, vi1.type()); - EXPECT_EQ(vi1.getInt(), 1); + auto vi = vInt.toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), 1); + + vi = vFloat.toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), 3); + + vi = vStr1.toInt(); + EXPECT_EQ(Value::Type::NULLVALUE, vi.type()); + + vi = vStr2.toInt(); + + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), 3); + + vi = vBool1.toInt(); + EXPECT_EQ(Value::Type::NULLVALUE, vi.type()); + + vi = vBool2.toInt(); + EXPECT_EQ(Value::Type::NULLVALUE, vi.type()); + + vi = vDate.toInt(); + EXPECT_EQ(Value::Type::NULLVALUE, vi.type()); + + vi = vNull.toInt(); + EXPECT_EQ(Value::Type::NULLVALUE, vi.type()); + + vi = vFloatMin.toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), std::numeric_limits::min()); - auto vi2 = vFloat.toInt(); - EXPECT_EQ(Value::Type::INT, vi2.type()); - EXPECT_EQ(vi2.getInt(), 3); + vi = vFloatMax.toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), std::numeric_limits::max()); - auto vi3 = vStr1.toInt(); - EXPECT_EQ(Value::Type::NULLVALUE, vi3.type()); + // string of int + vi = Value("123.").toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), 123); - auto vi4 = vStr2.toInt(); - EXPECT_EQ(Value::Type::INT, vi4.type()); - EXPECT_EQ(vi4.getInt(), 3); + vi = Value("-9223372036854775807").toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), std::numeric_limits::min() + 1); - auto vi5 = vBool1.toInt(); - EXPECT_EQ(Value::Type::NULLVALUE, vi5.type()); + vi = Value("-9223372036854775808").toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), std::numeric_limits::min()); - auto vi6 = vBool2.toInt(); - EXPECT_EQ(Value::Type::NULLVALUE, vi6.type()); + vi = Value("9223372036854775807").toInt(); + EXPECT_EQ(Value::Type::INT, vi.type()); + EXPECT_EQ(vi.getInt(), std::numeric_limits::max()); - auto vi7 = vDate.toInt(); - EXPECT_EQ(Value::Type::NULLVALUE, vi7.type()); + // String to int Overflow + vi = Value("9223372036854775808").toInt(); + EXPECT_EQ(Value::kNullOverflow, vi); - auto vi8 = vNull.toInt(); - EXPECT_EQ(Value::Type::NULLVALUE, vi8.type()); + vi = Value("-9223372036854775809").toInt(); + EXPECT_EQ(Value::kNullOverflow, vi); - auto vi9 = vFloatMin.toInt(); - EXPECT_EQ(Value::Type::INT, vi9.type()); - EXPECT_EQ(vi9.getInt(), std::numeric_limits::min()); + // Invlaid string + vi = Value("12abc").toInt(); + EXPECT_EQ(Value::kNullValue, vi); - auto vi10 = vFloatMax.toInt(); - EXPECT_EQ(Value::Type::INT, vi10.type()); - EXPECT_EQ(vi10.getInt(), std::numeric_limits::max()); + vi = Value("1.2abc").toInt(); + EXPECT_EQ(Value::kNullValue, vi); } } diff --git a/src/common/expression/SubscriptExpression.cpp b/src/common/expression/SubscriptExpression.cpp index a9578c06e..b34feb47f 100644 --- a/src/common/expression/SubscriptExpression.cpp +++ b/src/common/expression/SubscriptExpression.cpp @@ -120,26 +120,17 @@ void SubscriptExpression::accept(ExprVisitor *visitor) { // For the positive range bound it start from begin, // for the negative range bound it start from end const Value& SubscriptRangeExpression::eval(ExpressionContext &ctx) { - const auto listValue = DCHECK_NOTNULL(list_)->eval(ctx); + const auto& listValue = DCHECK_NOTNULL(list_)->eval(ctx); if (!listValue.isList()) { result_ = Value::kNullBadType; return result_; } - const auto list = listValue.getList(); + const auto& list = listValue.getList(); + size_t size = list.size(); int64_t lo, hi; if (lo_ == nullptr) { - auto hiValue = DCHECK_NOTNULL(hi_)->eval(ctx); - if (hiValue.isNull()) { - result_ = Value::kNullValue; - return result_; - } - if (!hiValue.isInt()) { - result_ = Value::kNullBadType; - return result_; - } - hi = hiValue.getInt(); - lo = (hi < 0 ? -list.size() : 0); - } else if (hi_ == nullptr) { + lo = 0; + } else { auto loValue = DCHECK_NOTNULL(lo_)->eval(ctx); if (loValue.isNull()) { result_ = Value::kNullValue; @@ -150,57 +141,47 @@ const Value& SubscriptRangeExpression::eval(ExpressionContext &ctx) { return result_; } lo = loValue.getInt(); - hi = (lo < 0 ? -1 : list.size()); + if (lo < 0) { + lo += size; + } + } + + if (hi_ == nullptr) { + hi = size; } else { - auto loValue = lo_->eval(ctx); - auto hiValue = hi_->eval(ctx); - if (loValue.isNull() || hiValue.isNull()) { + auto hiValue = DCHECK_NOTNULL(hi_)->eval(ctx); + if (hiValue.isNull()) { result_ = Value::kNullValue; return result_; } - if (!(loValue.isInt() && hiValue.isInt())) { + if (!hiValue.isInt()) { result_ = Value::kNullBadType; return result_; } - lo = loValue.getInt(); hi = hiValue.getInt(); + if (hi < 0) { + hi += size; + } } - // fit the out-of-range + // Out-of-bound slices are simply truncated if (lo < 0) { - if (lo < -static_cast(list.size())) { - lo = -list.size(); - } - } else { - if (lo >= static_cast(list.size())) { - result_ = List(); - return result_; - } + lo = 0; } - - if (hi < 0) { - if (hi < -static_cast(list.size())) { - result_ = List(); - return result_; - } - } else { - if (hi >= static_cast(list.size())) { - hi = list.size(); - } + if (lo >= hi) { + result_ = List(); + return result_; } - - const auto begin = lo < 0 ? list.values.end() + lo : list.values.begin() + lo; - const auto end = hi < 0 ? list.values.end() + hi : list.values.begin() + hi; - using iterCategory = typename std::iterator_traits::iterator_category; - static_assert(std::is_base_of_v); - if (std::distance(begin, end) < 0) { - // The unclosed range + if (static_cast(lo) >= size) { result_ = List(); return result_; } + if (static_cast(hi) >= size) { + hi = size; + } List r; - r.values.insert(r.values.end(), begin, end); + r.values = {list.values.begin()+lo, list.values.begin()+hi}; result_ = std::move(r); return result_; } diff --git a/src/common/expression/test/SubscriptExpressionTest.cpp b/src/common/expression/test/SubscriptExpressionTest.cpp index a58ff698c..0df8f2613 100644 --- a/src/common/expression/test/SubscriptExpressionTest.cpp +++ b/src/common/expression/test/SubscriptExpressionTest.cpp @@ -301,6 +301,15 @@ TEST_F(SubscriptExpressionTest, ListSubscriptRange) { EXPECT_TRUE(value.isList()); EXPECT_EQ(List(), value.getList()); } + // [0,1,2,3,4,5][-2..] => [4,5] + { + auto *lo = ConstantExpression::make(&pool, -2); + auto *hi = ConstantExpression::make(&pool, 3); + auto expr = SubscriptRangeExpression::make(&pool, list->clone(), lo, hi); + auto value = Expression::eval(expr, gExpCtxt); + EXPECT_TRUE(value.isList()); + EXPECT_EQ(List(), value.getList()); + } } TEST_F(SubscriptExpressionTest, MapSubscript) {