Skip to content

fix: parquet table#446

Open
iaojnh wants to merge 10 commits into
alibaba:mainfrom
iaojnh:fix/parquet-table
Open

fix: parquet table#446
iaojnh wants to merge 10 commits into
alibaba:mainfrom
iaojnh:fix/parquet-table

Conversation

@iaojnh
Copy link
Copy Markdown
Collaborator

@iaojnh iaojnh commented Jun 2, 2026

fix some bugs for parquet hash table.

@iaojnh iaojnh changed the title Fix/parquet table fix: parquet table Jun 2, 2026
return ParquetBufferContextHandle(buffer_id, arrow);
} else {
// Drop the empty entry inserted by operator[] on the failed load path.
table_.erase(buffer_id);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里的清理逻辑有点怪,acquire接口最好传递临时context,而不是table_[buffer_id]?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

并且acquire接口都声明为private

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_block_acquired也声明为private

}
}

std::shared_ptr<arrow::ChunkedArray> ParquetBufferPool::set_block_acquired(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_block_acquired可以直接传入context,少一次map get

if (context.ref_count.compare_exchange_weak(
current_count, current_count + 1, std::memory_order_acq_rel,
std::memory_order_acquire)) {
if (current_count == 0) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里为啥不直接把上面的current_count >=0 变为 current_count > 0 ? 另外current_count可能是负数吗?

ParquetBufferContext &context = table_[buffer_id];
ParquetBufferContext &context = iter->second;
int expected = 0;
if (context.ref_count.compare_exchange_strong(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个逻辑也可以去掉了

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants