lidavidm commented on code in PR #34227:
URL: https://github.com/apache/arrow/pull/34227#discussion_r1204003583
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/AceroSubstraitConsumer.java:
##########
@@ -124,8 +126,6 @@ private ArrowReader execute(ByteBuffer plan, Map<String,
ArrowReader> namedTable
} finally {
try {
AutoCloseables.close(arrowArrayStream);
- } catch (RuntimeException e) {
- throw e;
} catch (Exception e) {
throw new RuntimeException(e);
Review Comment:
Same here.
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/AceroSubstraitConsumer.java:
##########
@@ -96,15 +96,17 @@ private ArrowReader execute(String plan, Map<String,
ArrowReader> namedTables) t
ArrowArrayStream streamOutput =
ArrowArrayStream.allocateNew(this.allocator)
) {
String[] mapTableToMemoryAddress =
getMapTableToMemoryAddress(namedTables, arrowArrayStream);
- JniWrapper.get().executeSerializedPlanNamedTables(
+ JniWrapper.get().executeSerializedPlan(
plan,
mapTableToMemoryAddress,
streamOutput.memoryAddress()
);
return Data.importArrayStream(this.allocator, streamOutput);
} finally {
- for (ArrowArrayStream stream : arrowArrayStream) {
- stream.close();
+ try {
+ AutoCloseables.close(arrowArrayStream);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
Review Comment:
This method is declared to throw Exception already. Why are we catching and
re-throwing this?
##########
java/dataset/src/test/java/org/apache/arrow/dataset/substrait/TestAceroSubstraitConsumer.java:
##########
@@ -227,9 +208,6 @@ public void testRunBinaryQueryNamedTableNation() throws
Exception {
}
private static String planReplaceLocalFileURI(String plan, String uri) {
- StringBuilder builder = new StringBuilder(plan);
- builder.replace(builder.indexOf("FILENAME_PLACEHOLDER"),
- builder.indexOf("FILENAME_PLACEHOLDER") +
"FILENAME_PLACEHOLDER".length(), uri);
- return builder.toString();
+ return plan.replace("FILENAME_PLACEHOLDER", uri);
Review Comment:
Again: why is this a whole method? Just inline it; it's only used once.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
