Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct issue 2009 which the timestamp default value for edge not treat. #2038

Merged
merged 5 commits into from
Apr 2, 2020
Merged

Correct issue 2009 which the timestamp default value for edge not treat. #2038

merged 5 commits into from
Apr 2, 2020

Conversation

Shylock-Hg
Copy link
Contributor

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

@Shylock-Hg Shylock-Hg linked an issue Apr 1, 2020 that may be closed by this pull request
@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Apr 1, 2020
@Shylock-Hg Shylock-Hg removed the ready-for-testing PR: ready for the CI test label Apr 1, 2020
@Shylock-Hg
Copy link
Contributor Author

TODO: Add cases to nebula/test

Copy link
Contributor

@laura-ding laura-ding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! The pr #1452 missed modify it.
I think this should be encapsulated as a function , create tag/edge/ alter schema are use it, there is too much redundant code and maintenance is easy to miss.

if (value->getType() != nebula::cpp2::Value::Type::timestamp) {
LOG(ERROR) << "Create Edge Failed: " << name
<< " type mismatch";
handleErrorCode(cpp2::ErrorCode::E_CONFLICT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here should be E_INVALID_PARM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep same with before.

@Shylock-Hg
Copy link
Contributor Author

Well done! The pr #1452 missed modify it.
I think this should be encapsulated as a function , create tag/edge/ alter schema are use it, there is too much redundant code and maintenance is easy to miss.

I had issue this in #2036 , so this bugfix PR won't do it.

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Apr 1, 2020
@codecov-io
Copy link

Codecov Report

Merging #2038 into master will decrease coverage by 0.17%.
The diff coverage is 92.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
- Coverage   86.90%   86.73%   -0.18%     
==========================================
  Files         636      641       +5     
  Lines       59819    61076    +1257     
==========================================
+ Hits        51984    52972     +988     
- Misses       7835     8104     +269     
Impacted Files Coverage Δ
src/common/base/Configuration.h 100.00% <ø> (ø)
src/common/utils/NebulaKeyUtils.cpp 100.00% <ø> (ø)
src/common/utils/NebulaKeyUtils.h 90.41% <ø> (ø)
src/common/utils/test/NebulaKeyUtilsTest.cpp 97.10% <ø> (ø)
src/graph/DescribeSpaceExecutor.h 0.00% <ø> (ø)
src/graph/FetchEdgesExecutor.h 0.00% <ø> (ø)
src/graph/FetchExecutor.cpp 87.77% <ø> (-0.53%) ⬇️
src/graph/FetchVerticesExecutor.h 0.00% <ø> (ø)
src/graph/GoExecutor.h 56.25% <ø> (ø)
src/graph/GraphService.h 100.00% <ø> (ø)
... and 197 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ddaf4a...b272af3. Read the comment docs.

@jude-zhu jude-zhu requested a review from laura-ding April 1, 2020 09:05
laura-ding
laura-ding previously approved these changes Apr 1, 2020
Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done.

src/meta/processors/schemaMan/CreateEdgeProcessor.cpp Outdated Show resolved Hide resolved
src/meta/processors/schemaMan/CreateTagProcessor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dutor dutor merged commit 4aa9217 into vesoft-inc:master Apr 2, 2020
jude-zhu pushed a commit to jude-zhu/nebula that referenced this pull request Apr 2, 2020
…at. (vesoft-inc#2038)

* Correct issue 2009 which the timestamp default value for edge not treat.

* Add the cover cases.

* Fix one typo.

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit 4aa9217)
dutor pushed a commit that referenced this pull request Apr 2, 2020
* remove bigint because we dont support it (#2031)

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit 3423df1)

* fix crash (#2042)

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit eea7407)

* Sampling apply to all edges starting from a vertex. (#2013)

* Sampling from all edges.

* Update test for sampling multi edges.

* Testing both inbound and outbound.

* Fix clang compiling problem.

* Update the sampling.

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit 8810ecc)

* Fix the error when vertex/edge not exist if update. (#2025)

* Fix the error when vertex/edge not exist if update.

* Optimize the atomic error logical.

* Fix one alignment.

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit 9fc4e7f)

* Correct issue 2009 which the timestamp default value for edge not treat. (#2038)

* Correct issue 2009 which the timestamp default value for edge not treat.

* Add the cover cases.

* Fix one typo.

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
(cherry picked from commit 4aa9217)

Co-authored-by: flyingcat <1004815462@qq.com>
Co-authored-by: laura-ding <48548375+laura-ding@users.noreply.github.com>
Co-authored-by: CPWstatic <13495049+CPWstatic@users.noreply.github.com>
Co-authored-by: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com>
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
…at. (vesoft-inc#2038)

* Correct issue 2009 which the timestamp default value for edge not treat.

* Add the cover cases.

* Fix one typo.

Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Jan 31, 2023
vesoft-inc#2038)

* Enhance MemoryStats to avoid the false sharing effect on the CPU cache

* small rename

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

Co-authored-by: kyle.cao <kyle.cao@vesoft.com>
Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insert edge failed, error: Type Conversion Failed
5 participants