You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "waruto210 (via GitHub)" <gi...@apache.org> on 2025/11/06 05:43:26 UTC
[PR] GH-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
waruto210 opened a new pull request, #48073:
URL: https://github.com/apache/arrow/pull/48073
### Rationale for this change
Use the correct offset when accessing boolean values from the buffer.
### What changes are included in this PR?
In `unmaterialized_table_internal.h`, use the correct offset when accessing boolean values from the buffer. Also, add a unit test to regress this bug.
### Are these changes tested?
Yes
### Are there any user-facing changes?
No
**This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
Yes, fix a bug that caused incorrect data
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-5915249479
CI failures are ones on main, so I'll merge
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-3889490462
Sorry for my late check... There are CI failures. Could you rebase on main again?
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic merged PR #48073:
URL: https://github.com/apache/arrow/pull/48073
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-3495183177
:warning: GitHub issue #48072 **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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-5889407319
I've rebased 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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #48073:
URL: https://github.com/apache/arrow/pull/48073#discussion_r4145280891
##########
cpp/src/arrow/acero/unmaterialized_table_internal.h:
##########
@@ -172,7 +172,12 @@ class UnmaterializedCompositeTable {
builder.UnsafeAppendNull();
return Status::OK();
}
- builder.UnsafeAppend(bit_util::GetBit(source->template GetValues<uint8_t>(1), row));
+
+ int64_t array_offset = (source->offset + row) / 8;
+ int64_t bit_offset = (source->offset + row) % 8;
+
+ builder.UnsafeAppend(arrow::bit_util::GetBit(
+ source->template GetValues<uint8_t>(1, array_offset), bit_offset));
Review Comment:
`ArrayData::GetValues` requires `absolute_offset == 0` for bit-packed buffers. Since `bit_util::GetBit` accepts the full bit offset, could we pass it the base buffer and apply the offset there instead? This keeps the accessor within its documented contract and avoids duplicating the byte/bit split.
```suggestion
const int64_t bit_offset =
source->offset + static_cast<int64_t>(row);
builder.UnsafeAppend(bit_util::GetBit(
source->template GetValues<uint8_t>(1, 0), bit_offset));
```
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-5919187203
@zanmato1984 Thanks for reviewing this!
@thisisnic Thanks for completing 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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "waruto210 (via GitHub)" <gi...@apache.org>.
waruto210 commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-3501104516
@westonpace PTAL
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-5911398928
Hi @kou , I'm taking a look. Thanks for letting me know.
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "waruto210 (via GitHub)" <gi...@apache.org>.
waruto210 commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-3753988809
> +1
>
> Could you rebase on main to run CI?
Rebase done.
--
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-48072: [C++][Acero] fix a bug of materialize boolean [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #48073:
URL: https://github.com/apache/arrow/pull/48073#issuecomment-5919607536
Thanks both for looking at this! I'm going to see if there are any other similar PRs that are almost complete but look like they have been forgotten that I can complete with very minimal AI effort to get them over the line. If it starts getting annoying, given I don't quite have the C++ skills to totally understand what I'm producing, just let me know and I can change approach. Will try not to take on any which need more than minor changes anyway.
--
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