Skip to content

Conversation

@sbinet
Copy link
Contributor

@sbinet sbinet commented Apr 9, 2019

No description provided.

@sbinet
Copy link
Contributor Author

sbinet commented Apr 9, 2019

PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartcarnie: could I pick your brain on this?
it seems to me this is a valid change, but I'd like to know whether I didn't overlook anything...
(all tests pass, but...)

@codecov-io
Copy link

codecov-io commented Apr 9, 2019

Codecov Report

Merging #4131 into master will decrease coverage by 12.25%.
The diff coverage is 97.05%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4131       +/-   ##
===========================================
- Coverage    87.8%   75.54%   -12.26%     
===========================================
  Files         745       61      -684     
  Lines       91468     4229    -87239     
  Branches     1252        0     -1252     
===========================================
- Hits        80309     3195    -77114     
+ Misses      11042      926    -10116     
+ Partials      117      108        -9
Impacted Files Coverage Δ
go/arrow/array/fixedsize_binarybuilder.go 91.02% <100%> (+0.23%) ⬆️
go/arrow/array/numericbuilder.gen.go 92.87% <100%> (+24.7%) ⬆️
go/arrow/array/booleanbuilder.go 73.61% <100%> (+0.75%) ⬆️
go/arrow/array/binarybuilder.go 89.65% <50%> (-1.92%) ⬇️
go/arrow/memory/memory_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/uint64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/int64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/float64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/memory/memory_amd64.go 28.57% <0%> (-14.29%) ⬇️
go/arrow/math/math_amd64.go 31.57% <0%> (-5.27%) ⬇️
... and 693 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 1ff1bca...d5b04bf. Read the comment docs.

@sbinet
Copy link
Contributor Author

sbinet commented Apr 10, 2019

PTAL.

@sbinet sbinet changed the title ARROW-5117: [Go] fix panic when nil or empty slices are appended to numeric builders ARROW-5117: [Go] fix panic when nil or empty slices are appended to builders Apr 10, 2019
@sbinet sbinet force-pushed the issue-5117 branch 2 times, most recently from d5b04bf to ad99baf Compare April 12, 2019 12:42
@sbinet
Copy link
Contributor Author

sbinet commented Apr 12, 2019

gentle ping

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

LGTM minus my comment about the testing, although you might want to get someone else's input on this since I am new to the code.

@sbinet
Copy link
Contributor Author

sbinet commented Apr 15, 2019

ping @alexandreyc @stuartcarnie

@alexandreyc
Copy link
Contributor

LGTM modulo my comment on the test name for string builder.

@sbinet
Copy link
Contributor Author

sbinet commented Apr 15, 2019

ok, thanks.
just waiting a couple of days for @stuartcarnie, to make sure I haven't overlooked anything in the logic of the "append" of arrays values (especially the exit-early path...) that could have adverse effects elsewhere.

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.

4 participants