Correct read time stats in ParquetPageSource

Original codepath measures time spent on parquetReader.nextBatch(), in which only prepares reader for next batch, instead of reading data.
trinodb · Dec 16, 2018 · 834b01c · 834b01c
1 parent 28ec8f4
commit 834b01c
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 9 deletions.
diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/parquet/HdfsParquetDataSource.java b/presto-hive/src/main/java/com/facebook/presto/hive/parquet/HdfsParquetDataSource.java
@@ -34,6 +34,7 @@ public class HdfsParquetDataSource
     private final String name;
     private final long size;
     private final FSDataInputStream inputStream;
+    private long readTimeNanos;
     private long readBytes;
     private final FileFormatDataSourceStats stats;
 
@@ -51,6 +52,12 @@ public final long getReadBytes()
         return readBytes;
     }
 
+    @Override
+    public long getReadTimeNanos()
+    {
+        return readTimeNanos;
+    }
+
     @Override
     public final long getSize()
     {
@@ -73,16 +80,20 @@ public final void readFully(long position, byte[] buffer)
     @Override
     public final void readFully(long position, byte[] buffer, int bufferOffset, int bufferLength)
     {
-        readInternal(position, buffer, bufferOffset, bufferLength);
         readBytes += bufferLength;
+
+        long start = System.nanoTime();
+        readInternal(position, buffer, bufferOffset, bufferLength);
+        long currentReadTimeNanos = System.nanoTime() - start;
+
+        readTimeNanos += currentReadTimeNanos;
+        stats.readDataBytesPerSecond(bufferLength, currentReadTimeNanos);
     }
 
     private void readInternal(long position, byte[] buffer, int bufferOffset, int bufferLength)
     {
         try {
-            long readStart = System.nanoTime();
             inputStream.readFully(position, buffer, bufferOffset, bufferLength);
-            stats.readDataBytesPerSecond(bufferLength, System.nanoTime() - readStart);
         }
         catch (PrestoException e) {
             // just in case there is a Presto wrapper or hook

diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSource.java b/presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSource.java
@@ -64,7 +64,6 @@ public class ParquetPageSource
 
     private int batchId;
     private boolean closed;
-    private long readTimeNanos;
     private final boolean useParquetColumnNames;
 
     public ParquetPageSource(
@@ -125,7 +124,7 @@ public long getCompletedBytes()
     @Override
     public long getReadTimeNanos()
     {
-        return readTimeNanos;
+        return parquetReader.getDataSource().getReadTimeNanos();
     }
 
     @Override
@@ -145,12 +144,8 @@ public Page getNextPage()
     {
         try {
             batchId++;
-            long start = System.nanoTime();
-
             int batchSize = parquetReader.nextBatch();
 
-            readTimeNanos += System.nanoTime() - start;
-
             if (closed || batchSize <= 0) {
                 close();
                 return null;

diff --git a/presto-parquet/src/main/java/com/facebook/presto/parquet/ParquetDataSource.java b/presto-parquet/src/main/java/com/facebook/presto/parquet/ParquetDataSource.java
@@ -21,6 +21,8 @@ public interface ParquetDataSource
 {
     long getReadBytes();
 
+    long getReadTimeNanos();
+
     long getSize();
 
     void readFully(long position, byte[] buffer);