Skip to content

ARROW-2153/ARROW-2160: [C++/Python] Fix decimal precision inference#1618

Closed
cpcloud wants to merge 7 commits into
apache:masterfrom
cpcloud:ARROW-2160
Closed

ARROW-2153/ARROW-2160: [C++/Python] Fix decimal precision inference#1618
cpcloud wants to merge 7 commits into
apache:masterfrom
cpcloud:ARROW-2160

Conversation

@cpcloud

@cpcloud cpcloud commented Feb 16, 2018

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread cpp/src/arrow/python/helpers.cc Outdated

@cpcloud cpcloud Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This requires ARROW-2145 to address.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to wait for ARROW-2145 or merge this independently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a couple more tests to add here

@cpcloud

cpcloud commented Feb 19, 2018

Copy link
Copy Markdown
Contributor Author

@wesm @pitrou I reintroduced boost regex for parsing the decimal number, to address ARROW-2153. We were using a fairly straightforward algorithm before that implemented the regular expression by hand but it didn't allow numbers like 1e1. The regular expression to match that would be very complex and hard to read if written by hand so I decided to use boost regex. I didn't go with STL library because it doesn't support named capture groups, which I think make the code much more readable.

@cpcloud cpcloud changed the title ARROW-2160: [C++/Python] Fix decimal precision inference ARROW-2153/ARROW-2160: [C++/Python] Fix decimal precision inference Feb 19, 2018
@wesm

wesm commented Feb 19, 2018

Copy link
Copy Markdown
Member

std::regex is totally broken in gcc 4.8.x (what we're using for conda/pip releases) AFAIK so using <regex> isn't even an option right now. When we get past gcc 4.8 it might be nice to use the STL regexen

@cpcloud

cpcloud commented Feb 19, 2018

Copy link
Copy Markdown
Contributor Author

Yep, I believe std::regex_match is implemented as return false; :(

@cpcloud

cpcloud commented Feb 19, 2018

Copy link
Copy Markdown
Contributor Author

In gcc 4.8 that is. 4.9 may be implemented, but I don't think it supports named capture groups.

@wesm

wesm commented Feb 21, 2018

Copy link
Copy Markdown
Member

needs rebase

@cpcloud

cpcloud commented Feb 23, 2018

Copy link
Copy Markdown
Contributor Author

Closed in favor of #1651

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.

2 participants