Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-10881: [C++] Fix EXC_BAD_ACCESS in PutSpaced #9097

Closed
wants to merge 5 commits into from

Conversation

xhochy
Copy link
Member

@xhochy xhochy commented Jan 4, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Jan 4, 2021

@emkornfield
Copy link
Contributor

Is it possible to add a unit test?

@xhochy
Copy link
Member Author

xhochy commented Jan 4, 2021

@github-actions autotune

@nealrichardson
Copy link
Contributor

@xhochy autotune isn't working at the moment--INFRA has blocked some of the Actions we use, including on that workflow. See https://issues.apache.org/jira/browse/INFRA-21239

Copy link
Contributor

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

It would indeed be nice if there was a test.

if (valid_bits != NULLPTR) {
PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
this->memory_pool()));
T* data_ = reinterpret_cast<T*>(buffer->mutable_data());
Copy link
Contributor

Choose a reason for hiding this comment

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

Underscore-terminated variable names are only for attributes. Can you use another name? (e.g. non_spaced_data)

int num_valid_values = num_values;
if (valid_bits != NULLPTR) {
PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
this->memory_pool()));
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems buffer will be deallocated at the end of this if block, but you're using its contents afterwards (when calling Put).

const T* data = src;
int num_valid_values = num_values;
if (valid_bits != NULLPTR) {
PARQUET_ASSIGN_OR_THROW(auto buffer, ::arrow::AllocateBuffer(num_values * sizeof(T),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concerns below.

return ::arrow::util::internal::SpacedExpand<T>(buffer, num_values, null_count,
valid_bits, valid_bits_offset);
} else {
return num_values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you call Decode in this branch?

@pitrou
Copy link
Contributor

pitrou commented Jan 5, 2021

CI tests are green on my fork (except for the usual Homebrew dependency install failures).

@pitrou pitrou closed this in 2cf3f25 Jan 5, 2021
@xhochy
Copy link
Member Author

xhochy commented Jan 5, 2021

@pitrou Thanks!

@xhochy xhochy deleted the ARROW-10881 branch January 5, 2021 20:13
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
Closes apache#9097 from xhochy/ARROW-10881

Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants