paul-rogers commented on code in PR #2875:
URL: https://github.com/apache/drill/pull/2875#discussion_r1463921977
##########
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/AbstractHashBinaryRecordBatch.java:
##########
@@ -1312,7 +1312,9 @@ private void cleanup() {
}
// clean (and deallocate) each partition, and delete its spill file
for (HashPartition partn : partitions) {
- partn.close();
+ if (partn != null) {
+ partn.close();
+ }
Review Comment:
The above is OK as a work-around. I wonder, however, where the code added a
null pointer to the partition list. That should never happen. If it does, it
should be fixed at the point where the null pointer is added to the list.
Fixing it here is incomplete: there are other places where we loop through the
list, and those will also fail.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashPartition.java:
##########
@@ -157,11 +162,11 @@ public HashPartition(FragmentContext context,
BufferAllocator allocator, Chained
.build(logger);
} catch (SchemaChangeException sce) {
throw new IllegalStateException("Unexpected Schema Change while creating
a hash table",sce);
- }
- this.hjHelper = semiJoin ? null : new HashJoinHelper(context, allocator);
- tmpBatchesList = new ArrayList<>();
- if (numPartitions > 1) {
- allocateNewCurrentBatchAndHV();
+ } catch (OutOfMemoryException oom) {
+ close();
Review Comment:
This call is _probably_ fine. However, the design is that if any operator
fails, the entire operator stack is closed. So, `close()` should be called by
the fragment executor. There is probably no harm in calling `close()` here, as
long as the `close()` method is safe to call twice.
If the fragment executor _does not_ call close when the failure occurs
during setup, then there is a bug since failing to call `close()` results in
just this kind of error.
--
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]
