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

support geo encoder #380

Merged
merged 7 commits into from
Nov 18, 2021
Merged

support geo encoder #380

merged 7 commits into from
Nov 18, 2021

Conversation

jievince
Copy link
Contributor

@jievince jievince commented Nov 3, 2021

No description provided.

@jievince jievince force-pushed the geo branch 17 times, most recently from 047f35c to e12c08a Compare November 9, 2021 09:15
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #380 (3aa394f) into master (4b55c93) will increase coverage by 0.30%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #380      +/-   ##
============================================
+ Coverage     63.43%   63.73%   +0.30%     
- Complexity      692      705      +13     
============================================
  Files            65       65              
  Lines          3274     3337      +63     
  Branches        459      471      +12     
============================================
+ Hits           2077     2127      +50     
- Misses          883      888       +5     
- Partials        314      322       +8     
Impacted Files Coverage Δ
.../java/com/vesoft/nebula/encoder/RowWriterImpl.java 43.38% <69.49%> (+4.08%) ⬆️
...ava/com/vesoft/nebula/encoder/NebulaCodecImpl.java 87.50% <100.00%> (+0.35%) ⬆️
.../com/vesoft/nebula/encoder/SchemaProviderImpl.java 71.95% <100.00%> (+1.06%) ⬆️

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 4b55c93...3aa394f. Read the comment docs.

@jievince jievince requested a review from Nicole00 November 10, 2021 03:25
List<List<Coordinate>> rings = new ArrayList<List<Coordinate>>();
rings.add(shell);
rings.add(hole);
geogPolygonVal.setGgVal(Geography.pgVal(new Polygon(rings)));
Copy link
Contributor

Choose a reason for hiding this comment

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

please add test for null geograph value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a need, because null geo value is no different from other types. And TestEncoder.java also just test null value of int64

Copy link
Contributor

Choose a reason for hiding this comment

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

It's necessary, it happens out of bound error before for null string value int the properties. So you should add test like there are three geography properties but one is null to avoid that problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@jievince jievince force-pushed the geo branch 3 times, most recently from 6daaf9d to 61a114a Compare November 11, 2021 09:09
@jievince jievince requested a review from Nicole00 November 11, 2021 09:48
Copy link
Contributor

@Nicole00 Nicole00 left a comment

Choose a reason for hiding this comment

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

LGTM

@Nicole00 Nicole00 merged commit f29e978 into vesoft-inc:master Nov 18, 2021
@jievince jievince deleted the geo branch November 23, 2021 06:52
Nicole00 pushed a commit that referenced this pull request Nov 23, 2021
* support geo encoder

* debug

* remove debug log

* let WKBWriter use machine byte order

* add null test for geo

* debug

* format code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants