You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Vishwanatha-HD (via GitHub)" <gi...@apache.org> on 2025/11/21 17:24:31 UTC
[PR] GH-48214 Fix Arrow Reader Internal logic to enable Parquet DB support… [arrow]
Vishwanatha-HD opened a new pull request, #48215:
URL: https://github.com/apache/arrow/pull/48215
… on s390x
### Rationale for this change
This PR is intended to enable Parquet DB support on Big-endian (s390x) systems. The fix in this PR fixes the "Arrow/Reader_internal" logic.
### What changes are included in this PR?
The fix includes changes to following file:
cpp/src/parquet/arrow/reader_internal.cc
### Are these changes tested?
Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.
### Are there any user-facing changes?
No.
GitHub main Issue link: https://github.com/apache/arrow/issues/48151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "Vishwanatha-HD (via GitHub)" <gi...@apache.org>.
Vishwanatha-HD commented on code in PR #48215:
URL: https://github.com/apache/arrow/pull/48215#discussion_r2564868498
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -851,7 +851,41 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool* pool,
std::shared_ptr<ChunkedArray> chunked_array;
RETURN_NOT_OK(
TransferBinary(reader, pool, field->WithType(binary_type), &chunked_array));
+#if ARROW_LITTLE_ENDIAN
ARROW_ASSIGN_OR_RAISE(*out, chunked_array->View(field->type()));
+#else
+ // Convert little-endian bytes from Parquet to native-endian HalfFloat
Review Comment:
@pitrou.. Thanks for your comments here.. Can I plan to implement this change in the next pass, since the present code change is working.. We want to enable the Apache/Arrow support on s390x as soon as possible since its blocking many of the OCP AI tests on IBM Z.. Thanks.. !!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214 Fix Arrow Reader Internal logic to enable Parquet DB support… [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-3563979569
:warning: GitHub issue #48214 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "Vishwanatha-HD (via GitHub)" <gi...@apache.org>.
Vishwanatha-HD commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-4711047635
Hi @kou..
I have rebased this PR onto the main. Please check.. Thanks..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "k8ika0s (via GitHub)" <gi...@apache.org>.
k8ika0s commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-3568391315
@Vishwanatha-HD
These little corners of the Arrow/Parquet bridge tend to hide the more “surprising” BE behaviors, so it’s always nice to see them getting attention.
My own s390x work didn’t touch `reader_internal.cc`, so I’m mostly reading this with the lens of “does this match the patterns I’ve seen on hardware.” The decimal min/max extraction you added looks straightforward, and from what I’ve observed, normalizing those integer-backed stats before they’re handed downstream makes a real difference on BE.
Same with the half-float swap: I’ve noticed that half-floats are one of the places where BE architectures drift quickest if the reader doesn’t explicitly re-LE them, so calling `FromLittleEndian` here feels like the safer side of the fence.
I don’t see any conflicts with what I’ve been doing in encode/decode land — just wanted to chime in and confirm the behavior you’re targeting here lines up with what I’ve seen when running the full Parquet → Arrow → Parquet round-trip paths on BE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "kiszk (via GitHub)" <gi...@apache.org>.
kiszk commented on code in PR #48215:
URL: https://github.com/apache/arrow/pull/48215#discussion_r3425963054
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -851,7 +851,41 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool* pool,
std::shared_ptr<ChunkedArray> chunked_array;
RETURN_NOT_OK(
TransferBinary(reader, pool, field->WithType(binary_type), &chunked_array));
+#if ARROW_LITTLE_ENDIAN
ARROW_ASSIGN_OR_RAISE(*out, chunked_array->View(field->type()));
+#else
+ // Convert little-endian bytes from Parquet to native-endian HalfFloat
Review Comment:
I also prefer to use @pitrou 's approach to make it simple to easily maintain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "Vishwanatha-HD (via GitHub)" <gi...@apache.org>.
Vishwanatha-HD commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-4612235062
@pitrou @kou @kiszk @zanmato1984
Hi All,
I know its been long time since I have my PRs opened.. Can you please help me with review and merging to the upstream. Is there anything more that you people want me to do it, I would be happy to work on it. Please suggest.
I have verified that with my fix all the 113 testcase passes without any issues.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "kiszk (via GitHub)" <gi...@apache.org>.
kiszk commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-4726381230
@Vishwanatha-HD The existing ReadFloat16Files test reads pre-generated .parquet files, so it only meaningfully validates the platform those files came from. Since this is a BE-specific byte-order fix, a write→read round-trip test that asserts on the actual Float16 bits would guard against regressions on all platforms.
Would a value-based round-trip like the following example under `cpp/src/parquet/arrow/arrow_reader_writer_test.cc` make the intent explicit?
```
TEST(TestArrowReaderAdHoc, Float16RoundTripValues) {
using ::arrow::util::Float16;
using ::arrow::HalfFloatArray;
using ::arrow::HalfFloatType;
// Pick values whose two bytes differ, so a missing byte-swap is detectable.
const std::vector<Float16> values = {
Float16(+1.0), // bits 0x3c00
Float16(-2.0), // bits 0xc000
Float16(+0.5), // bits 0x3800
Float16(-0.0), // bits 0x8000
Float16(65504.0) // bits 0x7bff (max finite)
};
// Build a HalfFloatArray from the raw bits (native endian).
::arrow::HalfFloatBuilder builder;
ASSERT_OK(builder.Append(values[0].bits()));
ASSERT_OK(builder.AppendNull());
for (size_t i = 1; i < values.size(); ++i) {
ASSERT_OK(builder.Append(values[i].bits()));
}
std::shared_ptr<::arrow::Array> array;
ASSERT_OK(builder.Finish(&array));
auto table = ::arrow::Table::Make(
::arrow::schema({::arrow::field("f16", ::arrow::float16())}), {array});
std::shared_ptr<Table> result;
ASSERT_NO_FATAL_FAILURE(
DoSimpleRoundtrip(table, /*use_threads=*/false,
/*row_group_size=*/table->num_rows(), {}, &result));
ASSERT_EQ(result->num_columns(), 1);
ASSERT_EQ(result->column(0)->type()->id(), ::arrow::Type::HALF_FLOAT);
auto chunk = checked_pointer_cast<HalfFloatArray>(result->column(0)->chunk(0));
ASSERT_TRUE(chunk->IsNull(1));
ASSERT_EQ(chunk->Value(0), values[0].bits());
for (size_t i = 1; i < values.size(); ++i) {
ASSERT_FALSE(chunk->IsNull(static_cast<int64_t>(i) + 1));
ASSERT_EQ(chunk->Value(static_cast<int64_t>(i) + 1), values[i].bits());
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #48215:
URL: https://github.com/apache/arrow/pull/48215#discussion_r2556081992
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -851,7 +851,41 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool* pool,
std::shared_ptr<ChunkedArray> chunked_array;
RETURN_NOT_OK(
TransferBinary(reader, pool, field->WithType(binary_type), &chunked_array));
+#if ARROW_LITTLE_ENDIAN
ARROW_ASSIGN_OR_RAISE(*out, chunked_array->View(field->type()));
+#else
+ // Convert little-endian bytes from Parquet to native-endian HalfFloat
Review Comment:
I would favor a different approach: turn `TransferBinary` into:
```c++
Status TransferBinary(RecordReader* reader, MemoryPool* pool,
const std::shared_ptr<Field>& logical_type_field,
std::function<Result<std::shared_ptr<Array>>(std::shared_ptr<Array>)> array_process,
std::shared_ptr<ChunkedArray>* out) {
```
such that the optional `array_process` is called for each chunk. If carefully coded, this will help limit memory consumption by disposing of old chunks while creating the new ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #48215:
URL: https://github.com/apache/arrow/pull/48215#issuecomment-4725351559
@kiszk Could you review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48214: [C++][Parquet] Fix Arrow Reader Internal logic to enable Parquet DB support on s390x [arrow]
Posted by "kiszk (via GitHub)" <gi...@apache.org>.
kiszk commented on code in PR #48215:
URL: https://github.com/apache/arrow/pull/48215#discussion_r3425968191
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -851,7 +851,41 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool* pool,
std::shared_ptr<ChunkedArray> chunked_array;
RETURN_NOT_OK(
TransferBinary(reader, pool, field->WithType(binary_type), &chunked_array));
+#if ARROW_LITTLE_ENDIAN
ARROW_ASSIGN_OR_RAISE(*out, chunked_array->View(field->type()));
+#else
+ // Convert little-endian bytes from Parquet to native-endian HalfFloat
+ std::vector<std::shared_ptr<::arrow::Array>> out_chunks;
+ out_chunks.reserve(chunked_array->num_chunks());
+
+ for (const auto& chunk : chunked_array->chunks()) {
+ auto fsb = std::static_pointer_cast<::arrow::FixedSizeBinaryArray>(chunk);
+ const int64_t n = fsb->length();
+
+ // Allocate buffer for native-endian uint16 values
+ ARROW_ASSIGN_OR_RAISE(auto data_buf,
+ ::arrow::AllocateBuffer(n * sizeof(uint16_t), pool));
+ auto* out16 = reinterpret_cast<uint16_t*>(data_buf->mutable_data());
+
+ // Copy and convert from little-endian (Parquet spec) to native-endian
+ for (int64_t i = 0; i < n; ++i) {
Review Comment:
Can we make it simpler w/o `memcpy()` like?
```
for (int64_t i = 0; i < n; ++i) {
// FromLittleEndian handles the load + byte-swap; .bits() gives the
// native-endian uint16 that Arrow's HalfFloatArray expects.
out16[i] = ::arrow::util::Float16::FromLittleEndian(fsb.GetValue(i)).bits();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org