Skip to content

ARROW-2503: [Python] Prevent trailing space character for string statistics - #1945

Closed
jneuff wants to merge 1 commit into
apache:masterfrom
jneuff:fix-trailing-space-in-string-statistics
Closed

ARROW-2503: [Python] Prevent trailing space character for string statistics#1945
jneuff wants to merge 1 commit into
apache:masterfrom
jneuff:fix-trailing-space-in-string-statistics

Conversation

@jneuff

@jneuffjneuff commented Apr 25, 2018

Copy link
Copy Markdown

The trailing space is added in parquet-cpp.pyarrow calls the function FormatStatValue which adds the trailing space (https://github.com/apache/parquet-cpp/blob/master/src/parquet/types.cc#L52).

https://issues.apache.org/jira/browse/PARQUET-1283 is about fixing this behavior. Once the corresponding PR is merged into parquet-cpp and pyarrow is synced, the test_parquet.py:test_parquet_column_statistics_api will break for str. This PR fixes that breakage.

@xhochy

Copy link
Copy Markdown
Member

I guess this depends on the PR in parquet-cpp as the statistics tests are failing?

@jneuff

Copy link
Copy Markdown
Author

Sorry, I was a bit to eager to open this PR. I want to solve it without depending on the PR in parquet-cpp.

I'll reiterate.

@jneuff
jneuffforce-pushed the fix-trailing-space-in-string-statistics branch from 8c54fb7 to 57cdb6fCompareApril 26, 2018 12:29
@jneuff

Copy link
Copy Markdown
Author

I updated the scope of this PR, see the description above.

@pitrou

Copy link
Copy Markdown
Member

@jneuff, could you rebase this PR so that we get a fresh CI build?

This accounts for the fixed behavior of FormatStatValue
as described in PARQUET-1283.
@xhochy
xhochyforce-pushed the fix-trailing-space-in-string-statistics branch from 57cdb6f to f702f18CompareMay 1, 2018 13:28
@pitrou

Copy link
Copy Markdown
Member

I opened a competing PR #1973 to get CI as soon as possible. Let's see which one ends first.

@xhochy

Copy link
Copy Markdown
Member

@pitrou you should also have the right to push to contributor branches on github, no need to open new PRs if it's only a rebase.

@pitrou

Copy link
Copy Markdown
Member

In the past I've messed up things while trying to push to contributor branches :-)

@pitrou

Copy link
Copy Markdown
Member

Also the goal is to get an AppVeyor build on my account.

@pitrou

Copy link
Copy Markdown
Member

AppVeyor build succeeded at https://ci.appveyor.com/project/pitrou/arrow/build/1.0.359

@pitroupitrou closed this in a3aaff5May 1, 2018
Sign up for freeto 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

@jneuff@xhochy@pitrou