ARROW-10426: [C++] Allow writing large strings to Parquet #8632
Conversation
|
Perhaps I should add tests on the C++ side. |
pitrou
force-pushed
the
ARROW-10426-parquet-large-binary
branch
2 times, most recently
from
d347413
to
dfb698e
Compare
|
@kou Is it normal that the MinGW tests enable Python? |
pitrou
force-pushed
the
ARROW-10426-parquet-large-binary
branch
from
dfb698e
to
dff7d33
Compare
|
Yes. It's normal. |
|
@kou Well, the Python tests sometimes seem to time out on MinGW... |
|
Umm, they may have a problem in finalization... |
| const auto large_array = ::arrow::ArrayFromJSON(large_type, json); | ||
| const auto narrow_array = ::arrow::ArrayFromJSON(narrow_type, json); | ||
|
|
||
| this->RoundTripSingleColumn(large_array, narrow_array, |
There was a problem hiding this comment.
is this intended to be two different arrays? same as above? maybe add a comment on what you are testing here? (lack of schema to read back?)
There was a problem hiding this comment.
I'll add a comment indeed.
| void PutBinaryArray(const ArrayType& array) { | ||
| const int64_t total_bytes = | ||
| array.value_offset(array.length()) - array.value_offset(0); | ||
| PARQUET_THROW_NOT_OK(sink_.Reserve(total_bytes + array.length() * sizeof(uint32_t))); |
There was a problem hiding this comment.
is a check for int32 overflow needed here here?
There was a problem hiding this comment.
I don't think so (these are int64_t calculations AFAICT). However, I should fix the XXX below :-)
| @@ -127,6 +129,21 @@ class PlainEncoder : public EncoderImpl, virtual public TypedEncoder<DType> { | |||
| } | |||
|
|
|||
| protected: | |||
| template <typename ArrayType> | |||
There was a problem hiding this comment.
i'm not very familiar with this code, could you let me know what these changes are intended to do?
There was a problem hiding this comment.
Put(const ::arrow::Array&) was hardwired for narrow binary arrays, we need to handle both narrow and large arrays, hence this small refactor.
| std::shared_ptr<Comparator> Comparator::Make(Type::type physical_type, | ||
| SortOrder::type sort_order, | ||
| int type_length) { | ||
| if (SortOrder::SIGNED == sort_order) { |
There was a problem hiding this comment.
is this just moving code?
There was a problem hiding this comment.
Yes, so as to put internal implementation details in the anonymous namespace.
pitrou
force-pushed
the
ARROW-10426-parquet-large-binary
branch
from
dff7d33
to
4dcff96
Compare
|
@emkornfield Any other concern? |
cpp/src/parquet/encoding.cc
Outdated
| array.value_offset(array.length()) - array.value_offset(0); | ||
| PARQUET_THROW_NOT_OK(sink_.Reserve(total_bytes + array.length() * sizeof(uint32_t))); | ||
|
|
||
| constexpr size_t kMaxByteArraySize = std::numeric_limits<uint32_t>::max(); |
There was a problem hiding this comment.
its not clear from the spec, but I think this expected to be signed:
Java appears to use a int (signed 32 bit integer)
There was a problem hiding this comment.
I've changed the code to check for the signed maximum. It will be safer in any case.
|
One concern with the length check, otherwise LGTM. |
Large strings are still read back as regular strings.
pitrou
force-pushed
the
ARROW-10426-parquet-large-binary
branch
from
4dcff96
to
81c95c4
Compare
Large strings are still read back as regular strings. Closes apache#8632 from pitrou/ARROW-10426-parquet-large-binary Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Large strings are still read back as regular strings.