comphead commented on code in PR #7000:
URL: https://github.com/apache/datafusion-comet/pull/7000#discussion_r4488372024
##########
spark/pom.xml:
##########
@@ -36,6 +36,9 @@ under the License.
<properties>
<!-- The only published module: the parent pom skips deploying every
module -->
<maven.deploy.skip>false</maven.deploy.skip>
+ <!-- Hive's test-only catalog integration still uses Guava at runtime.
Keep the version local
+ so Guava is not exposed by or packaged into the published Comet
artifact. -->
+ <guava.version>33.2.1-jre</guava.version>
Review Comment:
This re-pins Guava at `33.2.1-jre` here and in
`contrib/delta-spark/pom.xml`, while `main` is on `33.7.1-jre` after #7259, so
resolving the root `pom.xml` conflict by deleting the property would revert
that bump. Could the root keep `guava.version` with its `dependencyManagement`
entry moved to test scope? Then this pom could drop its local version, and the
delta pom, which already uses `${guava.version}`, would not need to change.
##########
spark/src/main/java/org/apache/spark/sql/comet/execution/shuffle/CometBypassMergeSortShuffleWriter.java:
##########
@@ -364,36 +362,19 @@ private long[]
writePartitionedData(ShuffleMapOutputWriter mapOutputWriter) thro
private void writePartitionedDataWithChannel(File file,
WritableByteChannelWrapper outputChannel)
throws IOException {
- boolean copyThrewException = true;
- try {
- FileInputStream in = new FileInputStream(file);
- try (FileChannel inputChannel = in.getChannel()) {
- Utils.copyFileStreamNIO(inputChannel, outputChannel.channel(), 0L,
inputChannel.size());
- copyThrewException = false;
- } finally {
- Closeables.close(in, copyThrewException);
- }
- } finally {
- Closeables.close(outputChannel, copyThrewException);
+ try (FileInputStream in = new FileInputStream(file);
+ WritableByteChannelWrapper ignored = outputChannel) {
Review Comment:
If `new FileInputStream(file)` throws, the second resource is never
initialized, so `outputChannel` is not closed, where the previous code and
Spark's `BypassMergeSortShuffleWriter` close it in a `finally`. Declaring the
channel first keeps that and the original close order:
```java
try (WritableByteChannelWrapper ignored = outputChannel;
FileInputStream in = new FileInputStream(file)) {
```
The window is small since `file.exists()` was checked just before. I have
not run it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
