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

MINOR: [C++] Use the array returned by TweakValidityBit #14221

Merged
merged 3 commits into from
Sep 26, 2022

Conversation

kshitij12345
Copy link
Contributor

TweakValidityBit returns a new Array so the calling function should use it.

std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
int64_t index, bool validity) {
auto data = array->data()->Copy();
if (data->buffers[0] == nullptr) {
data->buffers[0] = *AllocateBitmap(data->length);
bit_util::SetBitsTo(data->buffers[0]->mutable_data(), 0, data->length, true);
}
bit_util::SetBitTo(data->buffers[0]->mutable_data(), index, validity);
data->null_count = kUnknownNullCount;
// Need to return a new array, because Array caches the null bitmap pointer
return MakeArray(data);
}

@kshitij12345 kshitij12345 marked this pull request as ready for review September 23, 2022 17:29
@github-actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@kshitij12345 kshitij12345 changed the title [minor] [test] use the array returned by TweakValidityBit MINOR: [test] use the array returned by TweakValidityBit Sep 23, 2022
@kshitij12345 kshitij12345 changed the title MINOR: [test] use the array returned by TweakValidityBit MINOR: [C++] use the array returned by TweakValidityBit Sep 23, 2022
@kshitij12345
Copy link
Contributor Author

cc: @lidavidm @pitrou (original author of the test)

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@lidavidm lidavidm changed the title MINOR: [C++] use the array returned by TweakValidityBit MINOR: [C++] Use the array returned by TweakValidityBit Sep 26, 2022
@lidavidm lidavidm merged commit f941118 into apache:master Sep 26, 2022
@ursabot
Copy link

ursabot commented Sep 26, 2022

Benchmark runs are scheduled for baseline = cc76736 and contender = f941118. f941118 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️2.76% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f941118e ec2-t3-xlarge-us-east-2
[Failed] f941118e test-mac-arm
[Failed] f941118e ursa-i9-9960x
[Finished] f941118e ursa-thinkcentre-m75q
[Finished] cc767365 ec2-t3-xlarge-us-east-2
[Finished] cc767365 test-mac-arm
[Failed] cc767365 ursa-i9-9960x
[Finished] cc767365 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
TweakValidityBit returns a new Array so the calling function should use it.
https://github.com/apache/arrow/blob/6cc37cf2d1ba72c46b64fbc7ac499bd0d7296d20/cpp/src/arrow/testing/gtest_util.cc#L568-L579

Authored-by: kshitij12345 <kshitijkalambarkar@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
TweakValidityBit returns a new Array so the calling function should use it.
https://github.com/apache/arrow/blob/6cc37cf2d1ba72c46b64fbc7ac499bd0d7296d20/cpp/src/arrow/testing/gtest_util.cc#L568-L579

Authored-by: kshitij12345 <kshitijkalambarkar@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
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.

3 participants