Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Aug 2, 2018

The implementation for numbers uses C++ istringstream. This makes casting a bit lenient (it will probably accept whitespace).

This is a rewrite of #1387

@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #2362 into master will increase coverage by 2.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2362      +/-   ##
==========================================
+ Coverage   84.66%   86.87%   +2.21%     
==========================================
  Files         293      237      -56     
  Lines       45324    42705    -2619     
==========================================
- Hits        38372    37099    -1273     
+ Misses       6911     5606    -1305     
+ Partials       41        0      -41
Impacted Files Coverage Δ
cpp/src/arrow/compute/kernels/cast.cc 91.54% <100%> (+1.67%) ⬆️
cpp/src/arrow/compute/compute-test.cc 99.54% <100%> (+0.05%) ⬆️
rust/src/buffer.rs
rust/src/memory_pool.rs
go/arrow/type_traits_boolean.go
go/arrow/math/int64_sse4_amd64.go
go/arrow/array/boolean.go
go/arrow/math/int64_avx2_amd64.go
go/arrow/math/int64.go
rust/src/error.rs
... and 48 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 edfbf84...c7db1b0. Read the comment docs.

@pitrou pitrou force-pushed the ARROW-1491-cast-string-to-number branch from 433f513 to 79cc17a Compare August 2, 2018 13:59
Copy link
Contributor

Choose a reason for hiding this comment

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

I sometimes wonder what the meeting was like that decided names like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same thought :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a little more readable by use tolower?

Copy link
Member Author

Choose a reason for hiding this comment

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

tolower is locale-dependent.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

Thanks for this. Made a small comment about reusing type traits but otherwise OK by me

Copy link
Member

Choose a reason for hiding this comment

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

You could use enable_if_number<O> here from arrow/type_traits.h

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@pitrou pitrou force-pushed the ARROW-1491-cast-string-to-number branch from 79cc17a to 4ce3000 Compare August 6, 2018 09:36
@pitrou
Copy link
Member Author

pitrou commented Aug 6, 2018

Some weird failure occurred on the AppVeyor build that didn't occur on my AppVeyor account:
https://ci.appveyor.com/project/pitrou/arrow/build/1.0.570

@pitrou pitrou force-pushed the ARROW-1491-cast-string-to-number branch from 4ce3000 to 3297aa1 Compare August 6, 2018 12:04
@pitrou
Copy link
Member Author

pitrou commented Aug 6, 2018

I want to make sure the AppVeyor failure was sporadic so I've triggered another build by pushing again.

pitrou added 3 commits August 6, 2018 15:44
The implementation for numbers uses the C standard strto* functions.
This makes casting a bit lenient (it will accept whitespace).
@wesm wesm force-pushed the ARROW-1491-cast-string-to-number branch from 3297aa1 to c7db1b0 Compare August 6, 2018 19:44
@wesm
Copy link
Member

wesm commented Aug 6, 2018

rebased

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1

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.

4 participants