Skip to content

Conversation

@tianchen92
Copy link
Contributor

Related to ARROW-1875.
This is Java side implementation.

@codecov-io
Copy link

Codecov Report

Merging #4977 into integration_ints_as_strings will increase coverage by 2.13%.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           integration_ints_as_strings    #4977      +/-   ##
===============================================================
+ Coverage                        87.57%   89.71%   +2.13%     
===============================================================
  Files                             1002      667     -335     
  Lines                           142651    98283   -44368     
  Branches                          1418        0    -1418     
===============================================================
- Hits                            124929    88176   -36753     
+ Misses                           17360    10107    -7253     
+ Partials                           362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/json/converter.cc 90.05% <0%> (-1.76%) ⬇️
cpp/src/arrow/json/chunked-builder.cc 79.91% <0%> (-1.68%) ⬇️
r/src/recordbatch.cpp
r/R/Table.R
js/src/util/fn.ts
go/arrow/array/bufferbuilder.go
r/src/symbols.cpp
rust/datafusion/src/execution/projection.rs
rust/datafusion/src/execution/filter.rs
rust/arrow/src/csv/writer.rs
... and 328 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 dc2c054...20cc581. Read the comment docs.

@wesm
Copy link
Member

wesm commented Aug 1, 2019

I'm surprised that the integration tests pass. Is RapidJSON loose about parsing strings to numbers?

@tianchen92
Copy link
Contributor Author

Also surprised me, I thought it would only pass if all the implementations work is done. Since it not break travis, could we just merge this into master or hold it in integration branch?

@kou
Copy link
Member

kou commented Aug 3, 2019

Why does this pull request target to https://github.com/apache/arrow/tree/integration_ints_as_strings branch instead of master?
Should we use the branch? Can we remove the branch?

@emkornfield
Copy link
Contributor

@kou we thought this change would need coordination between multiple languages. It appears it might not. @tianchen92 do you want to try to make the pull request against master and see if the tests still pass?

@tianchen92
Copy link
Contributor Author

@kou we thought this change would need coordination between multiple languages. It appears it might not. @tianchen92 do you want to try to make the pull request against master and see if the tests still pass?

I think i will work fine in master, I just opened a new PR #5002, close this one and will reopen if needed.

@tianchen92 tianchen92 closed this Aug 4, 2019
@kou
Copy link
Member

kou commented Aug 4, 2019

@emkornfield Thanks for the answer. There is no problem if we do this intentionally. I thought that this is caused by error. Because we don't use a branch like this.
Sorry for missing a discussion for this branch usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants