You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "wForget (via GitHub)" <gi...@apache.org> on 2025/03/19 06:57:47 UTC
[PR] fix: check if handle has been initialized before closing [datafusion-comet]
wForget opened a new pull request, #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554
## Which issue does this PR close?
Part of #1553
## Rationale for this change
Check that the handle is initialized before closing to avoid native thread panic.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
before this:
after this:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2738917966
> Is it possible to add a unit test for this?
Do we need to add a error test case for this obvious anomaly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2735623971
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1554?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 57.19%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9df6109`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/9df6109d2f1af24899a8ded72fa53591ff588c67?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 90 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/1554?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...va/org/apache/comet/parquet/NativeBatchReader.java](https://app.codecov.io/gh/apache/datafusion-comet/pull/1554?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcomet%2Fparquet%2FNativeBatchReader.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jb21ldC9wYXJxdWV0L05hdGl2ZUJhdGNoUmVhZGVyLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/1554?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1554 +/- ##
============================================
+ Coverage 56.12% 57.19% +1.06%
+ Complexity 976 937 -39
============================================
Files 119 122 +3
Lines 11743 12141 +398
Branches 2251 2281 +30
============================================
+ Hits 6591 6944 +353
- Misses 4012 4032 +20
- Partials 1140 1165 +25
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1554?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2741056298
> > I just wanted to see in what condition the NativeBatchReader can be called after close has been called.
>
> The scenario I encountered was not NativeBatchReader called after close, but an exception occurred during NativeBatchReader initialization and close was called. You can see some information in the first screenshot of description of #1553
Ah. Thanks for clarifying
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#discussion_r2003770078
##########
common/src/main/java/org/apache/comet/parquet/NativeBatchReader.java:
##########
@@ -459,7 +459,9 @@ public void close() throws IOException {
importer = null;
}
nativeUtil.close();
- Native.closeRecordBatchReader(this.handle);
+ if (handle > 0) {
+ Native.closeRecordBatchReader(this.handle);
Review Comment:
nit: we refer to both `handle` and `this.handle`. I assume they are the same thing but it would be nice to make them consistent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#discussion_r2004696018
##########
common/src/main/java/org/apache/comet/parquet/NativeBatchReader.java:
##########
@@ -459,7 +459,9 @@ public void close() throws IOException {
importer = null;
}
nativeUtil.close();
- Native.closeRecordBatchReader(this.handle);
+ if (handle > 0) {
+ Native.closeRecordBatchReader(this.handle);
Review Comment:
Thanks, changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2740681759
> I just wanted to see in what condition the NativeBatchReader can be called after close has been called.
The scenario I encountered was not NativeBatchReader called after close, but an exception occurred during NativeBatchReader initialization and close was called. You can see some information in the first screenshot of description of #1553
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2740525922
> > Is it possible to add a unit test for this?
>
> Do we need to add a error test case for this obvious anomaly?
It's not necessary. I just wanted to see in what condition the NativeBatchReader can be called after close has been called. Just in case there is a fundamental error that we are overlooking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#discussion_r2003757965
##########
common/src/main/java/org/apache/comet/parquet/NativeBatchReader.java:
##########
@@ -459,7 +459,9 @@ public void close() throws IOException {
importer = null;
}
nativeUtil.close();
- Native.closeRecordBatchReader(this.handle);
+ if (handle > 0) {
+ Native.closeRecordBatchReader(this.handle);
+ }
Review Comment:
And should we set handle to zero after being closed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#issuecomment-2737287481
Is it possible to add a unit test for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: check if handle has been initialized before closing [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #1554:
URL: https://github.com/apache/datafusion-comet/pull/1554#discussion_r2004695663
##########
common/src/main/java/org/apache/comet/parquet/NativeBatchReader.java:
##########
@@ -459,7 +459,9 @@ public void close() throws IOException {
importer = null;
}
nativeUtil.close();
- Native.closeRecordBatchReader(this.handle);
+ if (handle > 0) {
+ Native.closeRecordBatchReader(this.handle);
+ }
Review Comment:
Thanks, that makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org