Repository: drill Updated Branches: refs/heads/master 6076cc643 -> 826fc5b9c
DRILL-3017: Safeguard against NPEs in RecordReader.cleanup()s Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/826fc5b9 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/826fc5b9 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/826fc5b9 Branch: refs/heads/master Commit: 826fc5b9c2a6f044101967a9a2e49b20af2dae76 Parents: 6076cc6 Author: vkorukanti <venki.koruka...@gmail.com> Authored: Sun May 10 15:31:58 2015 -0700 Committer: vkorukanti <venki.koruka...@gmail.com> Committed: Sun May 10 15:31:58 2015 -0700 ---------------------------------------------------------------------- .../drill/exec/store/hive/HiveRecordReader.java | 5 +++- .../compliant/CompliantTextRecordReader.java | 5 +++- .../columnreaders/ParquetRecordReader.java | 19 ++++++++++------ .../exec/store/parquet2/DrillParquetReader.java | 5 +++- .../exec/store/text/DrillTextRecordReader.java | 5 +++- .../java/org/apache/drill/BaseTestQuery.java | 24 +++++++++++++++++++- .../apache/drill/TestDisabledFunctionality.java | 11 +++++---- 7 files changed, 58 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java ---------------------------------------------------------------------- diff --git a/contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java b/contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java index 5394ee3..3c8b9ba 100644 --- a/contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java +++ b/contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java @@ -344,7 +344,10 @@ public class HiveRecordReader extends AbstractRecordReader { @Override public void cleanup() { try { - reader.close(); + if (reader != null) { + reader.close(); + reader = null; + } } catch (Exception e) { logger.warn("Failure while closing Hive Record reader.", e); } http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/CompliantTextRecordReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/CompliantTextRecordReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/CompliantTextRecordReader.java index b2af32d..254e0d8 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/CompliantTextRecordReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/text/compliant/CompliantTextRecordReader.java @@ -144,7 +144,10 @@ public class CompliantTextRecordReader extends AbstractRecordReader { @Override public void cleanup() { try { - reader.close(); + if (reader != null) { + reader.close(); + reader = null; + } } catch (IOException e) { logger.warn("Exception while closing stream.", e); } http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java index 58cf321..2f07fb3 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java @@ -455,17 +455,22 @@ public class ParquetRecordReader extends AbstractRecordReader { // enable this for debugging when it is know that a whole file will be read // limit kills upstream operators once it has enough records, so this assert will fail // assert totalRecordsRead == footer.getBlocks().get(rowGroupIndex).getRowCount(); - for (ColumnReader column : columnStatuses) { - column.clear(); + if (columnStatuses != null) { + for (ColumnReader column : columnStatuses) { + column.clear(); + } + columnStatuses.clear(); + columnStatuses = null; } - columnStatuses.clear(); codecFactory.close(); - for (VarLengthColumn r : varLengthReader.columns) { - r.clear(); + if (varLengthReader != null) { + for (VarLengthColumn r : varLengthReader.columns) { + r.clear(); + } + varLengthReader.columns.clear(); + varLengthReader = null; } - varLengthReader.columns.clear(); } - } http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java index 99ac19c..4e7d628 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java @@ -342,7 +342,10 @@ public class DrillParquetReader extends AbstractRecordReader { @Override public void cleanup() { try { - pageReadStore.close(); + if (pageReadStore != null) { + pageReadStore.close(); + pageReadStore = null; + } } catch (IOException e) { logger.warn("Failure while closing PageReadStore", e); } http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java index 0322f36..e25bd74 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordReader.java @@ -231,7 +231,10 @@ public class DrillTextRecordReader extends AbstractRecordReader { @Override public void cleanup() { try { - reader.close(); + if (reader != null) { + reader.close(); + reader = null; + } } catch (IOException e) { logger.warn("Exception closing reader: {}", e); } http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java index f8ec090..db1ed34 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java +++ b/exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java @@ -59,6 +59,10 @@ import com.google.common.base.Charsets; import com.google.common.base.Preconditions; import com.google.common.io.Resources; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; + public class BaseTestQuery extends ExecTest { private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseTestQuery.class); @@ -319,7 +323,7 @@ public class BaseTestQuery extends ExecTest { protected static void testNoResult(int interation, String query, Object... args) throws Exception { query = String.format(query, args); - logger.debug("Running query:\n--------------\n"+query); + logger.debug("Running query:\n--------------\n" + query); for (int i = 0; i < interation; i++) { List<QueryDataBatch> results = client.runQuery(QueryType.SQL, query); for (QueryDataBatch queryDataBatch : results) { @@ -364,6 +368,24 @@ public class BaseTestQuery extends ExecTest { test(getFile(file)); } + /** + * Utility method which tests given query produces a {@link UserException} and the exception message contains + * the given message. + * @param testSqlQuery Test query + * @param expectedErrorMsg Expected error message. + */ + protected static void errorMsgTestHelper(final String testSqlQuery, final String expectedErrorMsg) throws Exception { + UserException expException = null; + try { + test(testSqlQuery); + } catch (final UserException ex) { + expException = ex; + } + + assertNotNull("Expected a UserException", expException); + assertThat(expException.getMessage(), containsString(expectedErrorMsg)); + } + public static String getFile(String resource) throws IOException{ URL url = Resources.getResource(resource); if (url == null) { http://git-wip-us.apache.org/repos/asf/drill/blob/826fc5b9/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java b/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java index 9420170..adbf653 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java +++ b/exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java @@ -18,6 +18,7 @@ package org.apache.drill; import org.apache.drill.common.exceptions.UserException; import org.apache.drill.common.util.FileUtils; +import org.apache.drill.exec.work.ExecErrorConstants; import org.apache.drill.exec.work.foreman.SqlUnsupportedException; import org.apache.drill.exec.work.foreman.UnsupportedDataTypeException; import org.apache.drill.exec.work.foreman.UnsupportedFunctionException; @@ -355,13 +356,15 @@ public class TestDisabledFunctionality extends BaseTestQuery{ } } - @Test(expected = UserException.class) // DRILL-2848 + @Test // DRILL-2848 public void testDisableDecimalCasts() throws Exception { - test("select cast('1.2' as decimal(9, 2)) from cp.`employee.json` limit 1"); + final String query = "select cast('1.2' as decimal(9, 2)) from cp.`employee.json` limit 1"; + errorMsgTestHelper(query, ExecErrorConstants.DECIMAL_DISABLE_ERR_MSG); } - @Test(expected = UserException.class) // DRILL-2848 + @Test // DRILL-2848 public void testDisableDecimalFromParquet() throws Exception { - test("select * from cp.`parquet/decimal_dictionary.parquet`"); + final String query = "select * from cp.`parquet/decimal_dictionary.parquet`"; + errorMsgTestHelper(query, ExecErrorConstants.DECIMAL_DISABLE_ERR_MSG); } } \ No newline at end of file