GH-40431: [C++] Move key_hash/key_map/light_array related files to internal for prevent using by users by ZhangHuiGui · Pull Request #40484 · apache/arrow

ZhangHuiGui · 2024-03-12T10:30:06Z

Rationale for this change

These files expose implementation details and APIs that are not meant for third-party use. This PR explicitly marks them internal, which also avoids having them installed.

Are these changes tested?

By existing builds and tests.

Are there any user-facing changes?

No, except hiding some header files that were not supposed to be included externally.

GitHub Issue: [C++] Crashed at TempStack alloc when use Hashing32::HashBatch independently #40431

ZhangHuiGui · 2024-03-12T10:36:16Z

cc @kou , it's a temporary fix. And i haven't add ut for now. PTAL?

kou · 2024-03-12T20:33:14Z

Is this for hash_temp_buf and null_hash_temp_buf?

kou · 2024-03-12T20:33:33Z

Is this for null_indices_buf?

kou · 2024-03-12T20:35:12Z

Why do you want to do this in HashBatch() not HashMultiColumn() (that has a real allocation logic)?

Yes, the codes here are unreasonable. It's temporary codes and refactored.

kou · 2024-03-12T20:35:38Z

Is this safe?
I think that stack is invalid outside of this block.

Oh,,, i lost my mind!

kou · 2024-03-12T20:36:15Z

You want to pass nullptr for temp_stack for your use case, right?

Yes, i think if user want to use the HashBatch as an independent api and they may needn't care about the stack size.

kou · 2024-03-12T20:38:26Z

We have similar codes in Hashing64::HashBatch(). Can we avoid it?

kou · 2024-03-12T20:38:39Z

Suggested change
constint64_tbuffer_size() const { returnbuffer_size_; }
int64_tbuffer_size() const { returnbuffer_size_; }

kou · 2024-03-12T20:40:05Z

Can we provide RequiredSize(), EstimateSize() or something instead of providing this?

ZhangHuiGui · 2024-03-14T01:57:22Z

We have already added the kPadding in PaddedAllocationSize, it's unnecessary to add it again.

ZhangHuiGui · 2024-03-25T04:26:08Z

@kou how about this refactor?

kou · 2024-03-25T04:32:11Z

Can we use auto here?
Suggested change
constuint32_t alloc_batch_size = std::min(num_rows, max_batch_size);
constint64_t estimate_alloc_size = EstimateBatchStackSize<uint32_t>(alloc_batch_size);
constauto alloc_batch_size = std::min(num_rows, max_batch_size);
constauto estimate_alloc_size = EstimateBatchStackSize<uint32_t>(alloc_batch_size);

kou · 2024-03-25T04:34:57Z

Could you set nullptr to ctx->stack before this function is exited?

kou · 2024-03-25T04:35:19Z

Could you revert a needless change?

kou · 2024-03-25T04:39:25Z

Could you move this comment to CheckAllocSizeValid()?

kou · 2024-03-25T04:40:17Z

Could you return arrow::Status instead of void here?
TempVectorStack::alloc() will not be able to use it for now but Hashing32::HashBatch() can use it.

Yes, reasonable. Refactored!

kou · 2024-03-25T04:49:25Z

I think that we should receive additional allocation size instead of total new allocation size here:
Suggested change
ARROW_DCHECK_LE(estimate_alloc_size, buffer_size_)
ARROW_DCHECK_LE(top_ + alloc_size, buffer_size_)

kou · 2024-03-25T04:50:34Z

Can we use EstimateAllocSize() here?

kou · 2024-03-25T04:50:47Z

Do we need this?

kou · 2024-03-25T04:51:14Z

Suggested change
constint32_t batch_size = static_cast<int32_t>(arr->length());
constauto batch_size = static_cast<int32_t>(arr->length());

kou · 2024-03-25T04:56:58Z

Do we need to export this?

Actually, i want to unify the logic in Hashing32 and Hashing64. But seems unnecessary.

ZhangHuiGui · 2024-03-26T14:32:30Z

@kou Thank you for your review!
@westonpace PTAL? This is a refactoring job, in order to allow users in need to better use the HashBatch related API.

mapleFU · 2024-03-26T14:55:47Z

cc @zanmato1984 if you're interested in this

zanmato1984

The idea of generalizing the hashing APIs is nice. Some suggestions.

zanmato1984 · 2024-03-26T17:06:54Z

We probably should use ARROW_CHECK_OK here?

Yes, you're right. ARROW_DCHECK_OK seems not work in NDEBUG mode.

zanmato1984 · 2024-03-26T17:08:29Z

Maybe we can align all the function names to either XxxAllocSize or XxxAllocationSize?

zanmato1984 · 2024-03-26T17:18:01Z

Maybe it can be more clear if we combine these two lines to const uint32_t max_batch_size = std::min(num_rows, util::MiniBatch::kMiniBatchLength);.

zanmato1984 · 2024-03-26T17:23:36Z

The scope of these three variables doesn't have to be this function, right? We can put them into the if statement below?

Actually the alloc_size is used both in if and else statement, it's not suitable to move these three variables into if statement.

You are right, thanks.

zanmato1984 · 2024-03-26T17:27:00Z

Suggested change
std::shared_ptr<util::TempVectorStack> temp_stack(nullptr);
auto stack = ctx->stack;
std::unique_ptr<util::TempVectorStack> temp_stack(nullptr);

Point is you don't really have to set the temp_stack pointer into the ctx. Just a regular temp variable will do. So you don't have to clear ctx->stack at the end.

Yes, totally agree!

zanmato1984 · 2024-03-26T17:28:01Z

Suggested change
RETURN_NOT_OK(ctx->stack->CheckAllocOverflow(alloc_size));
RETURN_NOT_OK(stack->CheckAllocOverflow(alloc_size));

zanmato1984 · 2024-03-26T17:28:13Z

Suggested change
auto hash_temp_buf = util::TempVectorHolder<uint32_t>(ctx->stack, alloc_batch_size);
auto hash_temp_buf = util::TempVectorHolder<uint32_t>(stack, alloc_batch_size);

zanmato1984 · 2024-03-26T17:28:23Z

Suggested change
auto null_indices_buf = util::TempVectorHolder<uint16_t>(ctx->stack, alloc_batch_size);
auto null_indices_buf = util::TempVectorHolder<uint16_t>(stack, alloc_batch_size);

zanmato1984 · 2024-03-26T17:28:54Z

Suggested change
util::TempVectorHolder<uint32_t>(ctx->stack, alloc_batch_size);
util::TempVectorHolder<uint32_t>(stack, alloc_batch_size);

zanmato1984 · 2024-03-26T17:29:10Z

Suggested change
if (temp_stack) {
ctx->stack = nullptr;
}

zanmato1984 · 2024-03-26T17:34:21Z

Suggested change
temp_stack = std::make_shared<util::TempVectorStack>();
temp_stack = std::make_unique<util::TempVectorStack>();

ZhangHuiGui · 2024-03-27T14:05:20Z

@zanmato1984 Thank you very much for your suggestion, the code looks clearer!

zanmato1984 · 2024-03-27T15:26:24Z

I would suggest keeping the name max_batch_size. It carries the meaning of how many rows to process in each iteration. In addition, this name is used everywhere in hash join related code so keeping it may complies with existing code base more. Last, it doesn't seem to be in the same category of the following three alloc family variables - we can think any alloc variable is solely to make sure the stack is large enough.

zanmato1984 · 2024-03-27T15:32:28Z

Maybe we can use ASSERT_RAISES_WITH_MESSAGE to check the detailed error message.

Yes, i've considered this. But the message has some detail numbers which related with internal alloc size. This is not convenient for future maintenance (for example, if some variables that require stack allocation are removed in HashMultiColumn, this test will need to be modified).

Yeah, you are right. Thanks.

zanmato1984

Some minor suggestions.

zanmato1984 · 2024-03-27T15:39:28Z

Suggested change
//alloc stack by HashBatch internal
//HashBatch using internally allocated buffer.

zanmato1984 · 2024-03-27T15:41:33Z

Suggested change
//alloc stack overflow in HashBatch
//HashBatch using pre-allocated buffer of insufficient size raises stack overflow.

zanmato1984 · 2024-03-27T15:42:41Z

Suggested change
//alloc stack normally in HashBatch
//HashBatch using big enough pre-allocated buffer.

zanmato1984

My one last suggestion :)

zanmato1984 · 2024-03-27T17:15:20Z

Suggested change
return Status::Invalid("TempVectorStack alloc overflow. (Actual ", buffer_size_,
"Bytes, expect ", alloc_size, "Bytes)");
return Status::Invalid("TempVectorStack allocation overflow: capacity ", buffer_size_, ", current size ", top, ", attempt allocating ", alloc_size);

zanmato1984

My one last suggestion :)

ZhangHuiGui · 2024-03-28T01:44:26Z

My one last suggestion :)

Thanks!

felipecrv · 2024-03-28T18:39:51Z

If there is a possibility that ctx->stack is nullptr, then it's better to declare a TempVectorStack * parameter explicitly so the caller can allocate a stack with the right memory pool instead of this function internally relying on the global default_memory_pool(). Most calls would be passing ctx, ctx->stack except for the ones that for some reason don't have a stack in the context.

Thanks, you're right. HashBatch is used internally the way you said!

ZhangHuiGui · 2024-03-30T05:40:07Z

@pitrou PTAL!
The new commit include two things:

Move these codes(key_hash/key_map/light_array) to internal! Besides, seems it's unnecessary to use internal namespace for them. The purpose we want is just prevent user's calling/
Simple refactor to simplify some codes in TempVectorStack.

westonpace

I agree these files were meant to be internal. Thanks for cleaning this up :)

pitrou · 2024-04-02T12:40:10Z

@github-actions crossbow submit -g cpp

pitrou · 2024-04-02T13:08:13Z

@github-actions crossbow submit -g cpp

pitrou · 2024-04-02T14:41:39Z

@github-actions crossbow submit -g cpp

github-actions · 2024-04-02T14:44:09Z

Revision: 0722f88

Submitted crossbow builds: ursacomputing/crossbow @ actions-5141a1fc14

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-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
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

pitrou · 2024-04-02T16:01:41Z

CI failures are unrelated, I'll merge.

conbench-apache-arrow · 2024-04-03T08:47:16Z

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

There were no benchmark performance regressions. 🎉

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

… to internal for prevent using by users (apache#40484) ### Rationale for this change These files expose implementation details and APIs that are not meant for third-party use. This PR explicitly marks them internal, which also avoids having them installed. ### Are these changes tested? By existing builds and tests. ### Are there any user-facing changes? No, except hiding some header files that were not supposed to be included externally. * GitHub Issue: apache#40431 Lead-authored-by: ZhangHuiGui <hugo.zhang@openpie.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actionsBot added Component: C++ awaiting review Awaiting review labels Mar 12, 2024

kou reviewed Mar 12, 2024
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Mar 12, 2024

ZhangHuiGui force-pushed the try-fix-40431 branch from 560de84 to c5dd7beCompare March 13, 2024 09:20

ZhangHuiGui commented Mar 14, 2024
View reviewed changes

kou reviewed Mar 25, 2024
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 25, 2024

ZhangHuiGui requested a review from westonpace as a code owner March 26, 2024 14:26

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 26, 2024

ZhangHuiGui force-pushed the try-fix-40431 branch from 6903766 to 6cff6f2Compare March 26, 2024 14:50

zanmato1984 requested changes Mar 26, 2024
View reviewed changes

zanmato1984 reviewed Mar 26, 2024
View reviewed changes

zanmato1984 reviewed Mar 27, 2024
View reviewed changes

ZhangHuiGui force-pushed the try-fix-40431 branch from b8db90c to 8f262afCompare March 28, 2024 01:43

github-actionsBot added the awaiting committer review Awaiting committer review label Mar 28, 2024

felipecrv reviewed Mar 28, 2024
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 28, 2024

mark key_hash/key_map/light_array to internal and some simple refactor
2cc866f

ZhangHuiGui force-pushed the try-fix-40431 branch from b13eeb5 to 2cc866fCompare March 30, 2024 05:32

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 30, 2024

ZhangHuiGui changed the title ~~GH-40431: [C++] Try to check/alloc the TempVectorStack size as HashBatch needed~~GH-40431: [C++] Move key_hash/key_map/light_array related files to internal for prevent using by usersMar 30, 2024

westonpace approved these changes Apr 2, 2024
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 2, 2024

pitrou requested changes Apr 2, 2024
View reviewed changes

Comment threadcpp/src/arrow/acero/schema_util.h Outdated

This comment was marked as outdated.
Sign in to view

ZhangHuiGuiand others added 2 commits April 2, 2024 21:00

fix
e15d1e5

Avoid DCHECK in non-internal header file
5180559

This comment was marked as outdated.
Sign in to view

Merge branch 'main' into try-fix-40431
0722f88

pitrou merged commit 8163d02 into apache:mainApr 2, 2024

pitrou removed the awaiting merge Awaiting merge label Apr 2, 2024

pitrou mentioned this pull request Apr 2, 2024
[C++] Crashed at TempStack alloc when use Hashing32::HashBatch independently #40431
Closed

	constint64_tbuffer_size() const { returnbuffer_size_; }
	int64_tbuffer_size() const { returnbuffer_size_; }

-constuint32_t alloc_batch_size = std::min(num_rows, max_batch_size);
-constint64_t estimate_alloc_size = EstimateBatchStackSize<uint32_t>(alloc_batch_size);
+constauto alloc_batch_size = std::min(num_rows, max_batch_size);
+constauto estimate_alloc_size = EstimateBatchStackSize<uint32_t>(alloc_batch_size);

	ARROW_DCHECK_LE(estimate_alloc_size, buffer_size_)
	ARROW_DCHECK_LE(top_ + alloc_size, buffer_size_)

	constint32_t batch_size = static_cast<int32_t>(arr->length());
	constauto batch_size = static_cast<int32_t>(arr->length());

	std::shared_ptr<util::TempVectorStack> temp_stack(nullptr);
	auto stack = ctx->stack;
	std::unique_ptr<util::TempVectorStack> temp_stack(nullptr);

	RETURN_NOT_OK(ctx->stack->CheckAllocOverflow(alloc_size));
	RETURN_NOT_OK(stack->CheckAllocOverflow(alloc_size));

	auto hash_temp_buf = util::TempVectorHolder<uint32_t>(ctx->stack, alloc_batch_size);
	auto hash_temp_buf = util::TempVectorHolder<uint32_t>(stack, alloc_batch_size);

	auto null_indices_buf = util::TempVectorHolder<uint16_t>(ctx->stack, alloc_batch_size);
	auto null_indices_buf = util::TempVectorHolder<uint16_t>(stack, alloc_batch_size);

	util::TempVectorHolder<uint32_t>(ctx->stack, alloc_batch_size);
	util::TempVectorHolder<uint32_t>(stack, alloc_batch_size);

	temp_stack = std::make_shared<util::TempVectorStack>();
	temp_stack = std::make_unique<util::TempVectorStack>();

	//alloc stack by HashBatch internal
	//HashBatch using internally allocated buffer.

	//alloc stack overflow in HashBatch
	//HashBatch using pre-allocated buffer of insufficient size raises stack overflow.

	//alloc stack normally in HashBatch
	//HashBatch using big enough pre-allocated buffer.

	return Status::Invalid("TempVectorStack alloc overflow. (Actual ", buffer_size_,
	"Bytes, expect ", alloc_size, "Bytes)");
	return Status::Invalid("TempVectorStack allocation overflow: capacity ", buffer_size_, ", current size ", top, ", attempt allocating ", alloc_size);

Uh oh!

Conversation

ZhangHuiGui commented Mar 12, 2024• edited by github-actions BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

ZhangHuiGui commented Mar 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHuiGui commented Mar 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangHuiGui commented Mar 26, 2024

Uh oh!

mapleFU commented Mar 26, 2024

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ZhangHuiGui commented Mar 12, 2024•
edited by github-actions Bot
Loading

zanmato1984Mar 26, 2024•
edited
Loading