Skip to content

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

Merged
lidavidm merged 3 commits into
apache:masterfrom
kshitij12345:fix/minor/test
Sep 26, 2022
Merged

MINOR: [C++] Use the array returned by TweakValidityBit#14221
lidavidm merged 3 commits into
apache:masterfrom
kshitij12345:fix/minor/test

Conversation

@kshitij12345

@kshitij12345 kshitij12345 commented Sep 23, 2022

Copy link
Copy Markdown
Contributor

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

arrow/cpp/src/arrow/testing/gtest_util.cc

Lines 568 to 579 in 6cc37cf

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

github-actions Bot commented Sep 23, 2022

Copy link
Copy Markdown

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

kshitij12345 commented Sep 23, 2022

Copy link
Copy Markdown
Contributor Author

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

lidavidm
lidavidm approved these changes Sep 26, 2022
View reviewed changes

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

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

ursabot commented Sep 26, 2022

Copy link
Copy Markdown

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
@kshitij12345 @zagto
MINOR: [C++] Use the array returned by TweakValidityBit (apache#14221)
694838c 
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 <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kshitij12345 @ursabot @lidavidm