ARROW-6000: [Python] Add support for LargeString and LargeBinary types by pitrou · Pull Request #4927 · apache/arrow

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou · 2019-07-23T14:15:35Z

Also fix a bug in Take / Filter for large binary types.

kszucs · 2019-07-24T10:18:24Z

@ursabot build

codecov-io · 2019-07-29T20:29:44Z

Codecov Report

Merging #4927 into master will increase coverage by 0.43%.
The diff coverage is 93.43%.

@@ Coverage Diff @@## master #4927 +/- ##
==========================================
+ Coverage 87.5% 87.93% +0.43% 
==========================================
Files 998 908 -90 Lines 141869 133088 -8781 Branches 1418 1418 ==========================================
- Hits 124139 117036 -7103 + Misses 17368 16042 -1326 + Partials 362 10 -352

Impacted Files	Coverage Δ
cpp/src/arrow/testing/random.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/pretty_print.cc	`83.33% <ø> (ø)`	⬆️
cpp/src/arrow/python/helpers.h	`88.88% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/python/helpers.cc	`77.05% <0%> (-0.92%)`	⬇️
cpp/src/arrow/json/converter-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.78% <100%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/pretty_print-test.cc	`100% <100%> (ø)`	⬆️
... and 124 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 171c3f7...63b4dfa. Read the comment docs.

pitrou · 2019-07-30T09:57:48Z

Travis-CI: https://travis-ci.org/pitrou/arrow/builds/565380024

pitrou · 2019-07-31T09:16:48Z

@jorisvandenbossche Want to take a look?

jorisvandenbossche · 2019-07-31T12:39:06Z

Yes, was planning to do that, thanks for the ping!

I played a bit with the branch (mainly with large_string), some things I noticed (to be clear: I suppose most of them are expected to not yet working, so not wanting to say they should be fixed in this PR, but some items might be worth JIRA issues):

With dictionary_encode() is not working on the array (a = pa.array(['a', 'b', 'c'], pa.large_string()); a.dictionary_encode() gives NotImplementedError), but constructing a dictionary array manually with large string dictionary works OK. unique is also not implemented.
to_pandas is not yet working (both on Table or Array). to_pylist is working fine. Also not yet for parquet, but I don't know for sure that parquet has a type that could be used for this (so not sure it should be working).
Do we want to be able to cast string to large_string (or the other way around) ?

take is not working correctly (but also not raising that it is not supported), while it works correctly for normal string:

In [29]: a = pa.array(['a', 'b', 'c'], pa.large_string()) In [30]: a.take(pa.array([0, 2])) Out[30]: <pyarrow.lib.LargeStringArray object at 0x7f19719d40f8>
[
"",
""
]
In [31]: b = pa.array(['a', 'b', 'c'], pa.string())
In [32]: b.take(pa.array([0, 2]))
Out[32]: <pyarrow.lib.StringArray object at 0x7f19719cd2b0>
[
"a",
"c"
]

Will take a look at the actual code now.

pitrou · 2019-07-31T12:57:31Z

Ok, some answers:

dictionary encoding isn't implemented, that's expected.
to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?
casting from string to large_string and vice-versa: yes, I opened https://issues.apache.org/jira/browse/ARROW-6071 for that and will leave it to interested contributors
take not working correctly: that's unexpected; either it should work or raise NotImplementedError...

jorisvandenbossche · 2019-07-31T13:14:14Z

Ah, I see you already opened https://issues.apache.org/jira/browse/ARROW-6071 for the casting yesterday

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Yes, that's something I could look into.

jorisvandenbossche

I already went through the python part (need to run now), which looks good.

jorisvandenbossche · 2019-07-31T13:22:34Z

+ Create large variable-length binary type
+
+ This data type may not be supported by all Arrow implementations. Unless
+ you need to represent data larger than 2GB, you should prefer binary().


should we clarify this is about the total size of a single array?

Ah, forget, a single value bigger than 2GB of course implies an array that is bigger than 2GB .. ;), so the distinction does not matter much.

pitrou · 2019-07-31T13:39:15Z

Ok, I've pushed a fix for the take() issue. Thanks for noticing!

pitrou · 2019-08-01T09:29:15Z

Will merge soon if no more comments.

pitrou · 2019-08-01T09:49:48Z

 public:
 static constexpr Type::type type_id = Type::STRING;
 static constexpr bool is_utf8 = true;
+ using EquivalentBinaryType = BinaryType;


@fsaintjacques Any issue with the naming?

jorisvandenbossche · 2019-08-02T08:00:16Z

to_pandas: similar. Perhaps that's something you would like to tackle in a separate issue?

Have to see a bit with my priorities, but at least created an issue for it: https://issues.apache.org/jira/browse/ARROW-6115

pitrou mentioned this pull request Jul 26, 2019
ARROW-750: [Format] [C++] Add LargeBinary and LargeString types #4921
Closed

pitrou force-pushed the ARROW-6000-py-large-binary branch 2 times, most recently from 3bdd5c9 to 63b4dfaCompare July 29, 2019 15:44

pitrou force-pushed the ARROW-6000-py-large-binary branch from 63b4dfa to 022173eCompare July 30, 2019 08:12

pitrou changed the title ~~[WIP] ARROW-6000: [Python] Add support for LargeString and LargeBinary types~~ ARROW-6000: [Python] Add support for LargeString and LargeBinary types Jul 30, 2019

pitrou force-pushed the ARROW-6000-py-large-binary branch from 022173e to aaf009eCompare July 30, 2019 08:14

ARROW-6000: [Python] Add support for LargeString and LargeBinary types
9672ca4

pitrou force-pushed the ARROW-6000-py-large-binary branch from aaf009e to 9672ca4Compare July 30, 2019 08:44

jorisvandenbossche reviewed Jul 31, 2019
View reviewed changes

Fix Take on LargeBinary / LargeString data
0bb1b7f

pitrou commented Aug 1, 2019
View reviewed changes

pitrou closed this in eb73b96Aug 1, 2019

pitrou deleted the ARROW-6000-py-large-binary branch August 1, 2019 10:50

asfimport mentioned this pull request Aug 2, 2019
[Python] Expose LargeBinaryType and LargeStringType #22406
Closed