GH-39583: [C++] Fix the issue of ExecBatchBuilder when appending consecutive tail rows with the same id may exceed buffer boundary (for fixed size types)

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.

[zanmato1984]

Rationale for this change

#39583 is a subsequent issue of #32570 (fixed by #39234). The last issue and fixed only resolved var length types. It turns out fixed size types have the same issue.

What changes are included in this PR?

Do the same fix of #39234 for fixed size types.

Are these changes tested?

UT included.

Are there any user-facing changes?

• Closes: [C++] Segmentation fault in ExecBatchBuilder #39583

[zanmato1984]

Hi @pitrou , I wonder if you could help to review this PR as this is very similar to its predecessor #39234 reviewed by you. Much appreciate it!

[pitrou]

This was for boolean data, right? Is it ok to remove this?

[zanmato1984]

Yes. Fixed size types of length 0/1/2/4/8 are using element-wise copying, i.e., no "word-wise then tail-bytes" copying. They don't go to this method:

arrow/cpp/src/arrow/compute/light_array.cc

Lines 513 to 546 in 3acc2ea

	case0:
	CollectBits(source->buffers[1]->data(), source->offset, target->mutable_data(1),
	num_rows_before, num_rows_to_append, row_ids);
	break;
	case1:
	Visit(source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	target->mutable_data(1)[num_rows_before + i] = *ptr;
	});
	break;
	case2:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint16_t*>(ptr);
	});
	break;
	case4:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint32_t*>(ptr);
	});
	break;
	case8:
	Visit(
	source, num_rows_to_append, row_ids,
	[&](int i, constuint8_t* ptr, uint32_t num_bytes) {
	reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
	*reinterpret_cast<constuint64_t*>(ptr);
	});
	break;

So I think we can simplify this method by not specially dealing with boolean type, thus the added ARROW_DCHECK several lines above.

[pitrou]

@zanmato1984 Can you rebase on git main?

[zanmato1984]

@zanmato1984 Can you rebase on git main?

Sure. Done.

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 20d9a38

Submitted crossbow builds: ursacomputing/crossbow @ actions-56dd6fe473

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

[pitrou]

LGTM, thank you @zanmato1984 . The original code quality is quite bad but this improves things slightly.

[pitrou]

@raulcd If you do another RC, this would be a good candidate fix to add.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1dc3b81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 8 possible false positives for unstable benchmarks that are known to sometimes produce them.