You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zanmato1984 (via GitHub)" <gi...@apache.org> on 2025/01/23 03:16:31 UTC
[PR] GH45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
zanmato1984 opened a new pull request, #45336:
URL: https://github.com/apache/arrow/pull/45336
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
#45334
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
1. An all-mighty test case that can effectively reveal all the bugs mentioned in the issue;
2. Other than directly fixing the bugs (actually simply casting to 64-bit somewhere in the multiplication will do), I did some refinement to the buffer accessors of the row table:
1. `null_masks()` -> `null_masks(row_id)` which does overflow-safe indexing inside;
2. `is_null(row_id, col_pos)` which does overflow-safe indexing and directly gets the bit of the column;
3. `data(1)` -> `fixed_length_rows(row_id)` which first asserts the row table being fixed-length, then does overflow-safe indexing inside;
4. `data(2)` -> `var_length_rows()` which only asserts the row table being var-length. It is supposed to be paired by the `offsets()` (which is already 64-bit by #43389 ).
3. The AVX2 specializations are fixed individually by using 64-bit multiplication and indexing.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
Yes.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
6. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
None.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) 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). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926893882
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -477,14 +477,15 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
const int64_t* source_rows_permutation) {
int64_t num_source_rows = source.length();
- int64_t fixed_length = target->metadata().fixed_length;
+ uint32_t fixed_length = target->metadata().fixed_length;
// Permutation of source rows is optional. Without permutation all that is
// needed is memcpy.
//
if (!source_rows_permutation) {
- memcpy(target->mutable_data(1) + fixed_length * first_target_row_id, source.data(1),
- fixed_length * num_source_rows);
+ DCHECK_LE(first_target_row_id, std::numeric_limits<uint32_t>::max());
+ memcpy(target->mutable_fixed_length_rows(static_cast<uint32_t>(first_target_row_id)),
+ source.fixed_length_rows(/*row_id=*/0), fixed_length * num_source_rows);
} else {
// Row length must be a multiple of 64-bits due to enforced alignment.
Review Comment:
Yes this is true. But IMHO this is acceptable because we also have other auxiliary data structures to aim the hash join so I wouldn't say this is very bad.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926898879
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
Review Comment:
Sure, will do.
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
+ __m128i right_lo = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_lo, 3), 1);
+ __m128i right_hi = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_hi, 3), 1);
+ __m256i right = _mm256_set_m128i(right_hi, right_lo);
+ right = _mm256_and_si256(right, bit_in_right);
Review Comment:
Sure, will do.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2608761937
:warning: GitHub issue #45334 **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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1927382640
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
Review Comment:
Yes, definitely.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2616112263
Revision: f7df7a4ff587791e8f650bea7ef9a8cc97e96a01
Submitted crossbow builds: [ursacomputing/crossbow @ actions-6bfabdccd3](https://github.com/ursacomputing/crossbow/branches/all?query=actions-6bfabdccd3)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703200/job/36233163490)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702622/job/36233159094)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703067/job/36233162257)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703212/job/36233164056)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703415/job/36233164224)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702840/job/36233160343)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702963/job/36233161048)|
|test-cuda-cpp-ubuntu-20.04-cuda-11.2.2|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703235/job/36233163544)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702709/job/36233159670)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702554/job/36233159102)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702947/job/36233161018)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702849/job/36233160374)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703078/job/36233162288)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702846/job/36233160401)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702894/job/36233160917)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703278/job/36233163572)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703454/job/36233164031)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703062/job/36233162263)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703308/job/36233163878)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703049/job/36233162260)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/12992703224/job/36233164621)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702834/job/36233160316)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702992/job/36233161497)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/12992702969/job/36233160824)|
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926666179
##########
cpp/src/arrow/compute/row/row_internal.h:
##########
@@ -199,29 +199,44 @@ class ARROW_EXPORT RowTableImpl {
const RowTableMetadata& metadata() const { return metadata_; }
/// \brief The number of rows stored in the table
int64_t length() const { return num_rows_; }
- // Accessors into the table's buffers
- const uint8_t* data(int i) const {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->data();
- }
- return NULLPTR;
+
+ inline const uint8_t* null_masks(uint32_t row_id) const {
+ return data(0) + static_cast<int64_t>(row_id) * metadata_.null_masks_bytes_per_row;
}
- uint8_t* mutable_data(int i) {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->mutable_data();
- }
- return NULLPTR;
+ inline uint8_t* null_masks(uint32_t row_id) {
Review Comment:
Should it be called `mutable_null_masks`, for consistency with other helper methods below?
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
+ __m128i right_lo = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_lo, 3), 1);
+ __m128i right_hi = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_hi, 3), 1);
+ __m256i right = _mm256_set_m128i(right_hi, right_lo);
+ right = _mm256_and_si256(right, bit_in_right);
Review Comment:
Also perhaps a more general helper:
```c++
// Get null bits at `null_bit_id` as a vector of 32-bit ints
__m256i GetRowNullBitsInt32(const RowTableImpl& rows, uint32_t null_bit_id, __m256 row_index32);
```
##########
cpp/src/arrow/compute/row/row_internal.h:
##########
@@ -199,29 +199,44 @@ class ARROW_EXPORT RowTableImpl {
const RowTableMetadata& metadata() const { return metadata_; }
/// \brief The number of rows stored in the table
int64_t length() const { return num_rows_; }
- // Accessors into the table's buffers
- const uint8_t* data(int i) const {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->data();
- }
- return NULLPTR;
+
+ inline const uint8_t* null_masks(uint32_t row_id) const {
Review Comment:
Note that `inline` is implied when the method definition is inside the class declaration, there's no need to put it explicitly.
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -477,14 +477,15 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
const int64_t* source_rows_permutation) {
int64_t num_source_rows = source.length();
- int64_t fixed_length = target->metadata().fixed_length;
+ uint32_t fixed_length = target->metadata().fixed_length;
// Permutation of source rows is optional. Without permutation all that is
// needed is memcpy.
//
if (!source_rows_permutation) {
- memcpy(target->mutable_data(1) + fixed_length * first_target_row_id, source.data(1),
- fixed_length * num_source_rows);
+ DCHECK_LE(first_target_row_id, std::numeric_limits<uint32_t>::max());
Review Comment:
Were these size constraints already implied but not tested for?
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -477,14 +477,15 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
const int64_t* source_rows_permutation) {
int64_t num_source_rows = source.length();
- int64_t fixed_length = target->metadata().fixed_length;
+ uint32_t fixed_length = target->metadata().fixed_length;
// Permutation of source rows is optional. Without permutation all that is
// needed is memcpy.
//
if (!source_rows_permutation) {
- memcpy(target->mutable_data(1) + fixed_length * first_target_row_id, source.data(1),
- fixed_length * num_source_rows);
+ DCHECK_LE(first_target_row_id, std::numeric_limits<uint32_t>::max());
+ memcpy(target->mutable_fixed_length_rows(static_cast<uint32_t>(first_target_row_id)),
+ source.fixed_length_rows(/*row_id=*/0), fixed_length * num_source_rows);
} else {
// Row length must be a multiple of 64-bits due to enforced alignment.
Review Comment:
Unrelated, but that's an interesting piece of info. Doesn't it blow up memory use if there is a small number of very small columns?
##########
cpp/src/arrow/compute/row/encode_internal.cc:
##########
@@ -260,44 +260,40 @@ void EncoderInteger::Decode(uint32_t start_row, uint32_t num_rows,
col_prep.metadata().fixed_length == rows.metadata().fixed_length) {
DCHECK_EQ(offset_within_row, 0);
uint32_t row_size = rows.metadata().fixed_length;
- memcpy(col_prep.mutable_data(1), rows.data(1) + start_row * row_size,
- num_rows * row_size);
+ memcpy(col_prep.mutable_data(1), rows.fixed_length_rows(start_row),
+ static_cast<int64_t>(num_rows) * row_size);
} else if (rows.metadata().is_fixed_length) {
- uint32_t row_size = rows.metadata().fixed_length;
- const uint8_t* row_base =
- rows.data(1) + static_cast<RowTableImpl::offset_type>(start_row) * row_size;
- row_base += offset_within_row;
uint8_t* col_base = col_prep.mutable_data(1);
switch (col_prep.metadata().fixed_length) {
case 1:
for (uint32_t i = 0; i < num_rows; ++i) {
- col_base[i] = row_base[i * row_size];
+ col_base[i] = *rows.fixed_length_rows(start_row + i);
Review Comment:
Hmm... we're not adding `offset_within_row` anymore? Is it deliberate?
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -494,10 +495,13 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
int64_t num_words_per_row = fixed_length / sizeof(uint64_t);
for (int64_t i = 0; i < num_source_rows; ++i) {
int64_t source_row_id = source_rows_permutation[i];
+ DCHECK_LE(source_row_id, std::numeric_limits<uint32_t>::max());
Review Comment:
If that's always the case then are we wasting memory and CPU cache by having a 64-bit permutation array?
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
Review Comment:
I seem to have already seen this kind of code in other PRs, perhaps we can add helpers to make it more readable (perhaps in a `avx2_internal.h` header?):
```c++
inline std::pair<__m256i, __m256i> WidenInt32to64(__m256 x) {
__m256i x_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(x));
__m256i x_hi =
_mm256_cvtepi32_epi64(_mm256_extracti128_si256(x, 1));
return {x_lo, x_hi};
}
// Compute `x * factor + addend` in the 64-bit domain
inline __m256i MulAddInt64(__m256 x, __m256 factor, __m256 addend) {
return _mm256_add_epi64(addend, _mm256_mul_epi32(x, factor));
}
inline __m256i MulAddInt64(__m256 x, int64_t factor, int64_t addend) {
return MulAddInt64(x, _mm256_set1_epi64x(factor), _mm256_set1_epi64x(addend));
}
```
then the code above could look like:
```c++
auto {irow_right_lo, irow_right_hi} = WidenInt32to64(irow_right);
auto bit_id_lo = MulAddInt64(irow_right_lo, null_mask_num_bytes * 8, pos_after_encoding);
auto bit_id_hi = MulAddInt64(irow_right_hi, null_mask_num_bytes * 8, pos_after_encoding);
```
(and in an ideal future, this would be migrated to xsimd :))
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926335538
##########
cpp/src/arrow/acero/hash_join_node_test.cc:
##########
@@ -3449,5 +3449,98 @@ TEST(HashJoin, LARGE_MEMORY_TEST(BuildSideOver4GBVarLength)) {
num_batches_left * num_rows_per_batch_left * num_batches_right);
}
+// GH-45334: The right side (the build side) payload column of the matching rows' are
Review Comment:
I may be able to further enhance this test to be able to place the matching key over 4GB, will try.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2609536286
Thank you @pitrou . Please hold a bit. The CI failure seems related, meanwhile I'm still working on enhancing the added test to cover not only payload columns.
Will update and address your comment soon.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1927372076
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
Review Comment:
I found a lot of legacy code can take advantage of these helpers. But they might be too much to put into this PR. Can I do it in another PR?
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1930585358
##########
cpp/src/arrow/acero/swiss_join_avx2.cc:
##########
@@ -237,31 +238,16 @@ int RowArrayAccessor::VisitNulls_avx2(const RowTableImpl& rows, int column_id,
//
constexpr int kUnroll = 8;
- const uint8_t* null_masks = rows.null_masks();
- __m256i null_bits_per_row =
- _mm256_set1_epi32(8 * rows.metadata().null_masks_bytes_per_row);
- __m256i pos_after_encoding =
- _mm256_set1_epi32(rows.metadata().pos_after_encoding(column_id));
+ uint32_t pos_after_encoding = rows.metadata().pos_after_encoding(column_id);
for (int i = 0; i < num_rows / kUnroll; ++i) {
__m256i row_id = _mm256_loadu_si256(reinterpret_cast<const __m256i*>(row_ids) + i);
- __m256i bit_id = _mm256_mullo_epi32(row_id, null_bits_per_row);
- bit_id = _mm256_add_epi32(bit_id, pos_after_encoding);
- __m256i bytes = _mm256_i32gather_epi32(reinterpret_cast<const int*>(null_masks),
- _mm256_srli_epi32(bit_id, 3), 1);
- __m256i bit_in_word = _mm256_sllv_epi32(
- _mm256_set1_epi32(1), _mm256_and_si256(bit_id, _mm256_set1_epi32(7)));
- // `result` will contain one 32-bit word per tested null bit, either 0xffffffff if the
- // null bit was set or 0 if it was unset.
- __m256i result =
- _mm256_cmpeq_epi32(_mm256_and_si256(bytes, bit_in_word), bit_in_word);
- // NB: Be careful about sign-extension when casting the return value of
- // _mm256_movemask_epi8 (signed 32-bit) to unsigned 64-bit, which will pollute the
- // higher bits of the following OR.
- uint32_t null_bytes_lo = static_cast<uint32_t>(
- _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_castsi256_si128(result))));
- uint64_t null_bytes_hi =
- _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_extracti128_si256(result, 1)));
- uint64_t null_bytes = null_bytes_lo | (null_bytes_hi << 32);
+ __m256i null32 = GetNullBitInt32(rows, pos_after_encoding, row_id);
+ null32 = _mm256_cmpeq_epi32(null32, _mm256_set1_epi32(1));
+ uint32_t null32_lo =
+ _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_castsi256_si128(null32)));
+ uint32_t null32_hi =
+ _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_extracti128_si256(null32, 1)));
Review Comment:
Can probably reuse `Cmp32to8` here?
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926897584
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -477,14 +477,15 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
const int64_t* source_rows_permutation) {
int64_t num_source_rows = source.length();
- int64_t fixed_length = target->metadata().fixed_length;
+ uint32_t fixed_length = target->metadata().fixed_length;
// Permutation of source rows is optional. Without permutation all that is
// needed is memcpy.
//
if (!source_rows_permutation) {
- memcpy(target->mutable_data(1) + fixed_length * first_target_row_id, source.data(1),
- fixed_length * num_source_rows);
+ DCHECK_LE(first_target_row_id, std::numeric_limits<uint32_t>::max());
Review Comment:
We assume row id to be `uint32_t` (that means no more than 2^32 rows are allowed) almost everywhere, so this is implied. But there are still some places weirdly and unnecessarily using `int64_t` as row id, here included.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2614786752
> The R failures seem related. Will take a look.
Fixed.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926890861
##########
cpp/src/arrow/compute/row/row_internal.h:
##########
@@ -199,29 +199,44 @@ class ARROW_EXPORT RowTableImpl {
const RowTableMetadata& metadata() const { return metadata_; }
/// \brief The number of rows stored in the table
int64_t length() const { return num_rows_; }
- // Accessors into the table's buffers
- const uint8_t* data(int i) const {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->data();
- }
- return NULLPTR;
+
+ inline const uint8_t* null_masks(uint32_t row_id) const {
Review Comment:
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2617089499
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b8185605295e55b1dc8740684351403f1860d87f.
There were 8 benchmark results with an error:
- Commit Run on `test-mac-arm` at [2025-01-27 19:20:18Z](https://conbench.ursa.dev/compare/runs/52aa4673c2fa486b873393301f430490...ce172f06ceba4ce080c315bc15b98682/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06797e6f89c07c4680005b7275de1f32)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06797e70598373f78000b42ca781ac32)
- and 6 more (see the report linked below)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/36256780136) has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2609697645
Hi @pitrou . Hopefully the CI should be fixed now. And the test has been enhanced to the extent that can also discover the overflow in key columns (the position of the key in row table depends on the hashing algorithm so it took me a while to tune a matching key value that will locate on the higher address in the row table - note the `289339070` in the test). I tested it by reverting several places of the fix in my local and the overflow did happen.
I'm addressing your comments now. Will update soon.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926898374
##########
cpp/src/arrow/acero/swiss_join.cc:
##########
@@ -494,10 +495,13 @@ void RowArrayMerge::CopyFixedLength(RowTableImpl* target, const RowTableImpl& so
int64_t num_words_per_row = fixed_length / sizeof(uint64_t);
for (int64_t i = 0; i < num_source_rows; ++i) {
int64_t source_row_id = source_rows_permutation[i];
+ DCHECK_LE(source_row_id, std::numeric_limits<uint32_t>::max());
Review Comment:
We are. In fact I have another WIP branch trying to clean them up.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1927384255
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
Review Comment:
I made the helper in the other comment though, which is independent of the ones suggested in this comment. (I think these ones are more "basic" than that one which is more row table specific.)
--
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] GH45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2608759707
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926891507
##########
cpp/src/arrow/compute/row/encode_internal.cc:
##########
@@ -260,44 +260,40 @@ void EncoderInteger::Decode(uint32_t start_row, uint32_t num_rows,
col_prep.metadata().fixed_length == rows.metadata().fixed_length) {
DCHECK_EQ(offset_within_row, 0);
uint32_t row_size = rows.metadata().fixed_length;
- memcpy(col_prep.mutable_data(1), rows.data(1) + start_row * row_size,
- num_rows * row_size);
+ memcpy(col_prep.mutable_data(1), rows.fixed_length_rows(start_row),
+ static_cast<int64_t>(num_rows) * row_size);
} else if (rows.metadata().is_fixed_length) {
- uint32_t row_size = rows.metadata().fixed_length;
- const uint8_t* row_base =
- rows.data(1) + static_cast<RowTableImpl::offset_type>(start_row) * row_size;
- row_base += offset_within_row;
uint8_t* col_base = col_prep.mutable_data(1);
switch (col_prep.metadata().fixed_length) {
case 1:
for (uint32_t i = 0; i < num_rows; ++i) {
- col_base[i] = row_base[i * row_size];
+ col_base[i] = *rows.fixed_length_rows(start_row + i);
Review Comment:
Sorry this was a mistake. Addressed. Thank you for spotting 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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1926891133
##########
cpp/src/arrow/compute/row/row_internal.h:
##########
@@ -199,29 +199,44 @@ class ARROW_EXPORT RowTableImpl {
const RowTableMetadata& metadata() const { return metadata_; }
/// \brief The number of rows stored in the table
int64_t length() const { return num_rows_; }
- // Accessors into the table's buffers
- const uint8_t* data(int i) const {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->data();
- }
- return NULLPTR;
+
+ inline const uint8_t* null_masks(uint32_t row_id) const {
+ return data(0) + static_cast<int64_t>(row_id) * metadata_.null_masks_bytes_per_row;
}
- uint8_t* mutable_data(int i) {
- ARROW_DCHECK(i >= 0 && i < kMaxBuffers);
- if (ARROW_PREDICT_TRUE(buffers_[i])) {
- return buffers_[i]->mutable_data();
- }
- return NULLPTR;
+ inline uint8_t* null_masks(uint32_t row_id) {
Review Comment:
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2613995942
The R failures seem related. Will take a look.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1930611737
##########
cpp/src/arrow/acero/swiss_join_avx2.cc:
##########
@@ -237,31 +238,16 @@ int RowArrayAccessor::VisitNulls_avx2(const RowTableImpl& rows, int column_id,
//
constexpr int kUnroll = 8;
- const uint8_t* null_masks = rows.null_masks();
- __m256i null_bits_per_row =
- _mm256_set1_epi32(8 * rows.metadata().null_masks_bytes_per_row);
- __m256i pos_after_encoding =
- _mm256_set1_epi32(rows.metadata().pos_after_encoding(column_id));
+ uint32_t pos_after_encoding = rows.metadata().pos_after_encoding(column_id);
for (int i = 0; i < num_rows / kUnroll; ++i) {
__m256i row_id = _mm256_loadu_si256(reinterpret_cast<const __m256i*>(row_ids) + i);
- __m256i bit_id = _mm256_mullo_epi32(row_id, null_bits_per_row);
- bit_id = _mm256_add_epi32(bit_id, pos_after_encoding);
- __m256i bytes = _mm256_i32gather_epi32(reinterpret_cast<const int*>(null_masks),
- _mm256_srli_epi32(bit_id, 3), 1);
- __m256i bit_in_word = _mm256_sllv_epi32(
- _mm256_set1_epi32(1), _mm256_and_si256(bit_id, _mm256_set1_epi32(7)));
- // `result` will contain one 32-bit word per tested null bit, either 0xffffffff if the
- // null bit was set or 0 if it was unset.
- __m256i result =
- _mm256_cmpeq_epi32(_mm256_and_si256(bytes, bit_in_word), bit_in_word);
- // NB: Be careful about sign-extension when casting the return value of
- // _mm256_movemask_epi8 (signed 32-bit) to unsigned 64-bit, which will pollute the
- // higher bits of the following OR.
- uint32_t null_bytes_lo = static_cast<uint32_t>(
- _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_castsi256_si128(result))));
- uint64_t null_bytes_hi =
- _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_extracti128_si256(result, 1)));
- uint64_t null_bytes = null_bytes_lo | (null_bytes_hi << 32);
+ __m256i null32 = GetNullBitInt32(rows, pos_after_encoding, row_id);
+ null32 = _mm256_cmpeq_epi32(null32, _mm256_set1_epi32(1));
+ uint32_t null32_lo =
+ _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_castsi256_si128(null32)));
+ uint32_t null32_hi =
+ _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_extracti128_si256(null32, 1)));
Review Comment:
Indeed. Moved `Cmp32/64To8` to common header and used it here. Thank you.
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2608808557
Hi @pitrou , mind to take a look? 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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45336:
URL: https://github.com/apache/arrow/pull/45336#issuecomment-2616105273
@github-actions crossbow submit -g cpp
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 merged PR #45336:
URL: https://github.com/apache/arrow/pull/45336
--
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-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45336:
URL: https://github.com/apache/arrow/pull/45336#discussion_r1927370144
##########
cpp/src/arrow/compute/row/compare_internal_avx2.cc:
##########
@@ -64,14 +66,21 @@ uint32_t KeyCompare::NullUpdateColumnToRowImp_avx2(
irow_right =
_mm256_loadu_si256(reinterpret_cast<const __m256i*>(left_to_right_map) + i);
}
- __m256i bitid =
- _mm256_mullo_epi32(irow_right, _mm256_set1_epi32(null_mask_num_bytes * 8));
- bitid = _mm256_add_epi32(bitid, _mm256_set1_epi32(null_bit_id));
- __m256i right =
- _mm256_i32gather_epi32((const int*)null_masks, _mm256_srli_epi32(bitid, 3), 1);
- right = _mm256_and_si256(
- _mm256_set1_epi32(1),
- _mm256_srlv_epi32(right, _mm256_and_si256(bitid, _mm256_set1_epi32(7))));
+ __m256i irow_right_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(irow_right));
+ __m256i irow_right_hi =
+ _mm256_cvtepi32_epi64(_mm256_extracti128_si256(irow_right, 1));
+ __m256i bit_id_lo =
+ _mm256_mul_epi32(irow_right_lo, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ __m256i bit_id_hi =
+ _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+ bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+ bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);
+ __m128i right_lo = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_lo, 3), 1);
+ __m128i right_hi = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+ _mm256_srli_epi64(bit_id_hi, 3), 1);
+ __m256i right = _mm256_set_m128i(right_hi, right_lo);
+ right = _mm256_and_si256(right, bit_in_right);
Review Comment:
Done. I made the helper you suggested and put it in a common header. Other than this file, there is one more piece of code in swiss_join_avx2.cc can reuse it. Pretty nice.
I also made two helper functions `Cmp32/64To8` local in compare_internal_avx2.cc that also save some LOC.
--
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