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-18178: [Java] ArrowVectorIterator incorrectly closes Vectors #14534

Conversation

lwhite1
Copy link
Contributor

@lwhite1 lwhite1 commented Oct 27, 2022

Prevent vectors from being closed when ArrowVectorIterator closes in the case where the reuseVectorSchemaRoot flag is set to false.

Test in JDBCToArrowVectorIteratorTest was updated to cover this case.

Tests in UnreliableMetaDataTest were modified to manually close VectorSchemaRoots, preventing an unclosed buffer exception being thrown when the RootAllocator was closed.

@lwhite1
Copy link
Contributor Author

lwhite1 commented Oct 27, 2022

@lidavidm If you could review that would be helpful

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

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.

LGTM.

Is there somewhere in the Javadocs or prose docs that this behavior should be described? (That is: if the VSR is reused, closing the iterator closes the root as expected; else the caller is expected to manage all roots)

@lwhite1
Copy link
Contributor Author

lwhite1 commented Oct 28, 2022

LGTM.

Is there somewhere in the Javadocs or prose docs that this behavior should be described? (That is: if the VSR is reused, closing the iterator closes the root as expected; else the caller is expected to manage all roots)

Good catch. It is mentioned in the method javadoc for next(). I just pushed a commit that makes it explicit in the close() method.

@lidavidm lidavidm merged commit f57ad93 into apache:master Oct 28, 2022
@ursabot
Copy link

ursabot commented Oct 30, 2022

Benchmark runs are scheduled for baseline = 9f4c632 and contender = f57ad93. f57ad93 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 ⬇️0.0% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.9% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f57ad932 ec2-t3-xlarge-us-east-2
[Failed] f57ad932 test-mac-arm
[Finished] f57ad932 ursa-i9-9960x
[Finished] f57ad932 ursa-thinkcentre-m75q
[Finished] 9f4c632f ec2-t3-xlarge-us-east-2
[Failed] 9f4c632f test-mac-arm
[Finished] 9f4c632f ursa-i9-9960x
[Finished] 9f4c632f 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

@ursabot
Copy link

ursabot commented Oct 30, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

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

3 participants