[ 
https://issues.apache.org/jira/browse/DRILL-6832?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17001141#comment-17001141
 ] 

ASF GitHub Bot commented on DRILL-6832:
---------------------------------------

ihuzenko commented on pull request #1929: DRILL-6832: Remove the old 
"unmanaged" external sort
URL: https://github.com/apache/drill/pull/1929#discussion_r360371678
 
 

 ##########
 File path: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/BatchGroup.java
 ##########
 @@ -18,140 +18,300 @@
 package org.apache.drill.exec.physical.impl.xsort;
 
 import java.io.IOException;
+import java.io.InputStream;
+import java.util.Collection;
 import java.util.Iterator;
 import java.util.concurrent.TimeUnit;
 
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.common.expression.SchemaPath;
-import org.apache.drill.exec.cache.VectorAccessibleSerializable;
+import org.apache.drill.exec.cache.VectorSerializer;
+import org.apache.drill.exec.cache.VectorSerializer.Writer;
 import org.apache.drill.exec.memory.BufferAllocator;
-import org.apache.drill.exec.ops.OperatorContext;
+import org.apache.drill.exec.physical.impl.spill.SpillSet;
 import org.apache.drill.exec.record.BatchSchema;
 import org.apache.drill.exec.record.SchemaUtil;
 import org.apache.drill.exec.record.TransferPair;
 import org.apache.drill.exec.record.TypedFieldId;
 import org.apache.drill.exec.record.VectorAccessible;
 import org.apache.drill.exec.record.VectorContainer;
 import org.apache.drill.exec.record.VectorWrapper;
-import org.apache.drill.exec.record.WritableBatch;
 import org.apache.drill.exec.record.selection.SelectionVector2;
 import org.apache.drill.exec.record.selection.SelectionVector4;
-import org.apache.hadoop.fs.FSDataInputStream;
-import org.apache.hadoop.fs.FSDataOutputStream;
-import org.apache.hadoop.fs.FileSystem;
-import org.apache.hadoop.fs.Path;
 
 import org.apache.drill.shaded.guava.com.google.common.base.Stopwatch;
 
-public class BatchGroup implements VectorAccessible, AutoCloseable {
+/**
+ * Represents a group of batches spilled to disk.
+ * <p>
+ * The batches are defined by a schema which can change over time. When the 
schema changes,
+ * all existing and new batches are coerced into the new schema. Provides a
+ * uniform way to iterate over records for one or more batches whether
+ * the batches are in memory or on disk.
+ * <p>
+ * The <code>BatchGroup</code> operates in two modes as given by the two
+ * subclasses:
+ * <ul>
+ * <li>Input mode (@link InputBatchGroup): Used to buffer in-memory batches
+ * prior to spilling.</li>
+ * <li>Spill mode (@link SpilledBatchGroup): Holds a "memento" to a set
+ * of batches written to disk. Acts as both a reader and writer for
+ * those batches.</li>
+ */
+
+public abstract class BatchGroup implements VectorAccessible, AutoCloseable {
   static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(BatchGroup.class);
 
-  private VectorContainer currentContainer;
-  private SelectionVector2 sv2;
-  private int pointer = 0;
-  private FSDataInputStream inputStream;
-  private FSDataOutputStream outputStream;
-  private Path path;
-  private FileSystem fs;
-  private BufferAllocator allocator;
-  private int spilledBatches = 0;
-  private OperatorContext context;
-  private BatchSchema schema;
-
-  public BatchGroup(VectorContainer container, SelectionVector2 sv2, 
OperatorContext context) {
-    this.sv2 = sv2;
-    this.currentContainer = container;
-    this.context = context;
-  }
+  /**
+   * The input batch group gathers batches buffered in memory before
+   * spilling. The structure of the data is:
+   * <ul>
+   * <li>Contains a single batch received from the upstream (input)
+   * operator.</li>
+   * <li>Associated selection vector that provides a sorted
+   * indirection to the values in the batch.</li>
+   * </ul>
+   */
 
-  public BatchGroup(VectorContainer container, FileSystem fs, String path, 
OperatorContext context) {
-    currentContainer = container;
-    this.fs = fs;
-    this.path = new Path(path);
-    this.allocator = context.getAllocator();
-    this.context = context;
-  }
+  public static class InputBatch extends BatchGroup {
+    private final SelectionVector2 sv2;
+    private final long dataSize;
+
+    public InputBatch(VectorContainer container, SelectionVector2 sv2, 
BufferAllocator allocator, long dataSize) {
+      super(container, allocator);
+      this.sv2 = sv2;
+      this.dataSize = dataSize;
+    }
 
-  public SelectionVector2 getSv2() {
-    return sv2;
+    public SelectionVector2 getSv2() { return sv2; }
+
+    public long getDataSize() { return dataSize; }
+
+    @Override
+    public int getRecordCount() {
+      if (sv2 != null) {
+        return sv2.getCount();
+      } else {
+        return super.getRecordCount();
+      }
+    }
+
+    @Override
+    public int getNextIndex() {
+      int val = super.getNextIndex();
+      if (val == -1) {
+        return val;
+      }
+      return sv2.getIndex(val);
 
 Review comment:
   For other methods in the ```InputBatch```, the  _**sv2**_ field is checked 
for `null` before access. Is it always safe to skip the check here? 
 
----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> Remove old "unmanaged" sort implementation
> ------------------------------------------
>
>                 Key: DRILL-6832
>                 URL: https://issues.apache.org/jira/browse/DRILL-6832
>             Project: Apache Drill
>          Issue Type: Improvement
>    Affects Versions: 1.14.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>            Priority: Minor
>
> Several releases back Drill introduced a new "managed" external sort that 
> enhanced the sort operator's memory management. To be safe, at the time, the 
> new version was controlled by an option, with the ability to revert to the 
> old version.
> The new version has proven to be stable. The time has come to remove the old 
> version.
> * Remove the implementation in {{physical.impl.xsort}}.
> * Move the implementation from {{physical.impl.xsort.managed}} to the parent 
> package.
> * Remove the conditional code in the batch creator.
> * Remove the option that allowed disabling the new version.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to