Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1058#discussion_r154478289
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/BatchGroup.java
---
@@ -249,7 +247,7 @@ public void close() throws IOException {
ex = e;
}
try {
- closeOutputStream();
+ closeWriter();
--- End diff --
Just as a matter of record, this abstraction is rather awkward. It tries to
be three things: 1) a writer for a spill file, 2) a reader for a spill file and
3) the tombstone for the spill file.
It would be much better to have separate reader and writer classes that
come and go, with this class just representing the spill file over its
lifetime. There was, however, no easy way to make that change and preserve
existing code. Since I'd already changed more code than I really wanted, I left
well enough alone. (See the "unmanaged" version for the original code, which
was even more murky.)
Now that you've modified the Writer to be the batch writer, I wonder if we
should revisit the issue rather than preserving the old, muddy, semantics of
this class.
---
