pitrou commented on code in PR #50271:
URL: https://github.com/apache/arrow/pull/50271#discussion_r3542701663
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -725,13 +725,66 @@ class PARQUET_NO_EXPORT FixedSizeListReader : public
ListReader<int32_t> {
DCHECK_EQ(data->buffers.size(), 2);
DCHECK_EQ(field()->type()->id(), ::arrow::Type::FIXED_SIZE_LIST);
const auto& type =
checked_cast<::arrow::FixedSizeListType&>(*field()->type());
- const int32_t* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
- for (int x = 1; x <= data->length; x++) {
- int32_t size = offsets[x] - offsets[x - 1];
- if (size != type.list_size()) {
- return Status::Invalid("Expected all lists to be of size=",
type.list_size(),
- " but index ", x, " had size=", size);
+ const auto* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
+ const int32_t list_size = type.list_size();
+ const uint8_t* valid_bits =
+ data->buffers[0] != nullptr ? data->buffers[0]->data() : nullptr;
+ auto validate_offsets = [&](int64_t start, int64_t length,
+ int32_t expected_size) -> Status {
+ std::span<const int32_t> run_offsets(offsets + start,
+ static_cast<size_t>(length + 1));
+ const auto first_invalid_offset = std::ranges::adjacent_find(
+ run_offsets,
+ [&](int32_t left, int32_t right) { return right - left !=
expected_size; });
+ if (first_invalid_offset != run_offsets.end()) {
+ const int64_t x =
+ start + std::ranges::distance(run_offsets.begin(),
first_invalid_offset);
+ return Status::Invalid("Expected offset at index ", x + 1, " to be ",
+ offsets[x] + expected_size, " but got ",
offsets[x + 1]);
}
+ return Status::OK();
+ };
+ if (valid_bits != nullptr) {
Review Comment:
Why not `data->GetNullCount() != 0`? This will remove the motivation for
`needs_padding`.
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -725,13 +725,66 @@ class PARQUET_NO_EXPORT FixedSizeListReader : public
ListReader<int32_t> {
DCHECK_EQ(data->buffers.size(), 2);
DCHECK_EQ(field()->type()->id(), ::arrow::Type::FIXED_SIZE_LIST);
const auto& type =
checked_cast<::arrow::FixedSizeListType&>(*field()->type());
- const int32_t* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
- for (int x = 1; x <= data->length; x++) {
- int32_t size = offsets[x] - offsets[x - 1];
- if (size != type.list_size()) {
- return Status::Invalid("Expected all lists to be of size=",
type.list_size(),
- " but index ", x, " had size=", size);
+ const auto* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
+ const int32_t list_size = type.list_size();
+ const uint8_t* valid_bits =
+ data->buffers[0] != nullptr ? data->buffers[0]->data() : nullptr;
+ auto validate_offsets = [&](int64_t start, int64_t length,
+ int32_t expected_size) -> Status {
+ std::span<const int32_t> run_offsets(offsets + start,
+ static_cast<size_t>(length + 1));
+ const auto first_invalid_offset = std::ranges::adjacent_find(
Review Comment:
I'm not sure C++20 ranges are available on all our CI platforms
unfortunately (some of them require old compilers).
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -725,13 +725,66 @@ class PARQUET_NO_EXPORT FixedSizeListReader : public
ListReader<int32_t> {
DCHECK_EQ(data->buffers.size(), 2);
DCHECK_EQ(field()->type()->id(), ::arrow::Type::FIXED_SIZE_LIST);
const auto& type =
checked_cast<::arrow::FixedSizeListType&>(*field()->type());
- const int32_t* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
- for (int x = 1; x <= data->length; x++) {
- int32_t size = offsets[x] - offsets[x - 1];
- if (size != type.list_size()) {
- return Status::Invalid("Expected all lists to be of size=",
type.list_size(),
- " but index ", x, " had size=", size);
+ const auto* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
+ const int32_t list_size = type.list_size();
+ const uint8_t* valid_bits =
+ data->buffers[0] != nullptr ? data->buffers[0]->data() : nullptr;
+ auto validate_offsets = [&](int64_t start, int64_t length,
+ int32_t expected_size) -> Status {
+ std::span<const int32_t> run_offsets(offsets + start,
+ static_cast<size_t>(length + 1));
+ const auto first_invalid_offset = std::ranges::adjacent_find(
+ run_offsets,
+ [&](int32_t left, int32_t right) { return right - left !=
expected_size; });
+ if (first_invalid_offset != run_offsets.end()) {
+ const int64_t x =
+ start + std::ranges::distance(run_offsets.begin(),
first_invalid_offset);
+ return Status::Invalid("Expected offset at index ", x + 1, " to be ",
+ offsets[x] + expected_size, " but got ",
offsets[x + 1]);
}
+ return Status::OK();
+ };
+ if (valid_bits != nullptr) {
+ bool needs_padding = false;
Review Comment:
Since the code below is non-trivial, can you add comments to guide the
reader?
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -725,13 +725,66 @@ class PARQUET_NO_EXPORT FixedSizeListReader : public
ListReader<int32_t> {
DCHECK_EQ(data->buffers.size(), 2);
DCHECK_EQ(field()->type()->id(), ::arrow::Type::FIXED_SIZE_LIST);
const auto& type =
checked_cast<::arrow::FixedSizeListType&>(*field()->type());
- const int32_t* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
- for (int x = 1; x <= data->length; x++) {
- int32_t size = offsets[x] - offsets[x - 1];
- if (size != type.list_size()) {
- return Status::Invalid("Expected all lists to be of size=",
type.list_size(),
- " but index ", x, " had size=", size);
+ const auto* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
+ const int32_t list_size = type.list_size();
+ const uint8_t* valid_bits =
+ data->buffers[0] != nullptr ? data->buffers[0]->data() : nullptr;
+ auto validate_offsets = [&](int64_t start, int64_t length,
+ int32_t expected_size) -> Status {
+ std::span<const int32_t> run_offsets(offsets + start,
+ static_cast<size_t>(length + 1));
+ const auto first_invalid_offset = std::ranges::adjacent_find(
+ run_offsets,
+ [&](int32_t left, int32_t right) { return right - left !=
expected_size; });
+ if (first_invalid_offset != run_offsets.end()) {
+ const int64_t x =
+ start + std::ranges::distance(run_offsets.begin(),
first_invalid_offset);
+ return Status::Invalid("Expected offset at index ", x + 1, " to be ",
+ offsets[x] + expected_size, " but got ",
offsets[x + 1]);
Review Comment:
The previous error message was much more understandable than this, IMHO.
(the fact that we're first decoding to a variable-sized list with offsets is
an implementation detail)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
