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.

Sign up for GitHub

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

Jump to bottom

Optimize update logic & add a new strategy #673

Merged
merged 8 commits into from
Sep 24, 2019
Merged

Optimize update logic & add a new strategy #673

merged 8 commits into from
Sep 24, 2019

Conversation

imbajin
Copy link
Member

@imbajin imbajin commented Aug 30, 2019

  1. if original data is not null but new data is null, use original data instead of null
  2. add update strategy "OVERRIDE"

@codecov
Copy link

codecov bot commented Aug 30, 2019 •
edited
Loading

Codecov Report

Merging #673 into master will decrease coverage by <.01%.
The diff coverage is 70%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #673      +/-   ##
============================================
- Coverage      74.3%   74.29%   -0.01%     
  Complexity     4089     4089              
============================================
  Files           220      220              
  Lines         17917    17918       +1     
  Branches       2571     2571              
============================================
- Hits          13314    13313       -1     
- Misses         3247     3250       +3     
+ Partials       1356     1355       -1
Impacted Files Coverage Δ Complexity Δ
...u/hugegraph/schema/builder/PropertyKeyBuilder.java 87.61% <ø> (ø) 36 <0> (ø) ⬇️
...om/baidu/hugegraph/backend/store/BackendEntry.java 66.66% <ø> (ø) 1 <0> (ø) ⬇️
...n/java/com/baidu/hugegraph/schema/PropertyKey.java 83.15% <ø> (ø) 43 <0> (ø) ⬇️
...u/hugegraph/backend/store/mysql/MysqlSessions.java 69.61% <ø> (ø) 20 <0> (ø) ⬇️
...backend/store/postgresql/PostgresqlSerializer.java 84.61% <0%> (ø) 4 <0> (ø) ⬇️
...egraph/traversal/optimize/HugeScriptTraversal.java 78.04% <100%> (ø) 5 <0> (ø) ⬇️
...ugegraph/backend/serializer/SerializerFactory.java 86.2% <100%> (ø) 9 <0> (ø) ⬇️
...in/java/com/baidu/hugegraph/backend/id/EdgeId.java 77.52% <100%> (+0.25%) 33 <0> (ø) ⬇️
...ugegraph/backend/cache/CachedGraphTransaction.java 94% <50%> (ø) 29 <0> (ø) ⬇️
...gegraph/backend/cache/CachedSchemaTransaction.java 96.61% <50%> (ø) 28 <0> (ø) ⬇️
... and 3 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 efcd839...b9ac2c1. Read the comment docs.

javeme
javeme requested changes Aug 30, 2019
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/id/EdgeId.java Outdated
@@ -193,7 +193,8 @@ public static EdgeId parse(String id) throws NotFoundException {
String[] idParts = SplicingIdGenerator.split(id);
if (!(idParts.length == 4 || idParts.length == 5)) {
throw new NotFoundException("Edge id must be formatted as 4~5 parts"
+ ", but got '%s'", id);
+ ", but got %s parts, '%s'",
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer move "parts" to the next line

@javeme
Copy link
Contributor

javeme commented Sep 11, 2019

there are conflicting files, please rebase.
and also update client.

@imbajin imbajin force-pushed the enhance-update-strategy branch from 90b4c2f to ce91f2f Compare September 12, 2019 11:16
@imbajin
Optimize update logic & add a new strategy
e33b023 
1. if original data is not null but new data is null, use original data instead of null
2. add update strategy "OVERRIDE"
@imbajin
reverse equals to avoid NPE & add missing "Override"
ffe4b65
@imbajin
tiny fix
58ce865
@imbajin
enhance logic
4ce0e20
@imbajin imbajin force-pushed the enhance-update-strategy branch from e325830 to 4ce0e20 Compare September 16, 2019 07:49
javeme
javeme requested changes Sep 18, 2019
View reviewed changes
hugegraph-api/src/main/java/com/baidu/hugegraph/define/UpdateStrategy.java
@@ -139,6 +139,18 @@ Object updatePropertyValue(Object oldProperty, Object newProperty) {
void checkPropertyType(Object oldProperty, Object newProperty) {
this.checkCollectionType(oldProperty, newProperty);
}
},

OVERRIDE {
Copy link
Contributor

Choose a reason for hiding this comment

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

upgrade api version

Linary
Linary previously approved these changes Sep 18, 2019
View reviewed changes
hugegraph-api/src/main/java/com/baidu/hugegraph/license/LicenseVerifier.java
// NOTE: can't use JsonUtil due to it bind tinkerpop jackson
ObjectMapper mapper = new ObjectMapper();
try {
try (InputStream stream =
Copy link
Contributor

Choose a reason for hiding this comment

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

good improve!

@imbajin
unify some Uuid to UUID
b32fb29 
keep 'UuidID' & those impl from Tinkerpop as they are
zhoney
zhoney requested changes Sep 18, 2019
View reviewed changes
hugegraph-api/src/main/java/com/baidu/hugegraph/version/ApiVersion.java Outdated
* and split range to rangeInt, rangeFloat, rangeLong and rangeDouble
* [0.44] Issue-633: Support unique index
* [0.45] Issue-673: Add a new update strategy
Copy link
Contributor

Choose a reason for hiding this comment

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

make it clear:

Add OVERRIDE update strategy

@javeme javeme mentioned this pull request Sep 20, 2019
Merged
@javeme javeme merged commit 9d8ceb4 into apache:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Linary Linary Linary approved these changes

@javeme javeme javeme approved these changes

@zhoney zhoney zhoney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imbajin @javeme @Linary @zhoney