IMPALA-4097: Crash in kudu-scan-node-test The kudu-scan-node-test was calling GetNext() with a NULL row batch, which isn't valid. This wasn't failing until a recent code change, and only occasionally due to the timing of scanner threads producing row batches. One batch is empty and the other has 1 row. This test worked when the empty row batch was added to the batch queue first (which usually happened), but the test code couldn't handle the other ordering properly. This fixes the test to be more careful about what is being exercised.
Change-Id: I0e30964232ed137fce7a99bf7e9557293b5905c8 Reviewed-on: http://gerrit.cloudera.org:8080/4337 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/7b4a6fac Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/7b4a6fac Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/7b4a6fac Branch: refs/heads/master Commit: 7b4a6fac1acf16fb84b38a005f69c228620ddb3f Parents: d153d18 Author: Matthew Jacobs <[email protected]> Authored: Thu Sep 8 13:19:47 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Sat Sep 10 00:50:31 2016 +0000 ---------------------------------------------------------------------- be/src/exec/kudu-scan-node-test.cc | 27 ++++++++++++++++++++------- be/src/exec/kudu-scan-node.cc | 1 + 2 files changed, 21 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7b4a6fac/be/src/exec/kudu-scan-node-test.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-scan-node-test.cc b/be/src/exec/kudu-scan-node-test.cc index 972ae80..8324628 100644 --- a/be/src/exec/kudu-scan-node-test.cc +++ b/be/src/exec/kudu-scan-node-test.cc @@ -312,19 +312,32 @@ TEST_F(KuduScanNodeTest, TestScanEmptyString) { vector<TScanRangeParams> params; CreateScanRangeParams(num_cols_to_materialize, ¶ms); SetUpScanner(&scanner, params); - bool eos = false; RowBatch* batch = obj_pool_->Add( new RowBatch(scanner.row_desc(), DEFAULT_ROWS_PER_BATCH, mem_tracker_.get())); - for (int i = 0; i < 2 && batch->num_rows() == 0; ++i) { - // Allow for up to 2 empty row batches since there are scanners created for all - // tablets (1 split point), and only one row was inserted. + bool eos = false; + int total_num_rows = 0; + + // Need to call GetNext() a total of 3 times to allow for: + // 1) the row batch containing the single row + // 2) an empty row batch (since there are scanners created for both tablets) + // 3) a final call which returns eos. + // The first two may occur in any order and are checked in the for loop below. + for (int i = 0; i < 2; ++i) { ASSERT_OK(scanner.GetNext(runtime_state_.get(), batch, &eos)); + ASSERT_FALSE(eos); + if (batch->num_rows() > 0) { + total_num_rows += batch->num_rows(); + ASSERT_EQ(PrintBatch(batch), "[(10 null )]\n"); + } + batch->Reset(); } - ASSERT_EQ(1, batch->num_rows()); + ASSERT_EQ(1, total_num_rows); - ASSERT_OK(scanner.GetNext(runtime_state_.get(), NULL, &eos)); + // One last call to find the batch queue empty and GetNext() returns eos. + ASSERT_OK(scanner.GetNext(runtime_state_.get(), batch, &eos)); ASSERT_TRUE(eos); - ASSERT_EQ(PrintBatch(batch), "[(10 null )]\n"); + ASSERT_EQ(0, batch->num_rows()); + scanner.Close(runtime_state_.get()); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7b4a6fac/be/src/exec/kudu-scan-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-scan-node.cc b/be/src/exec/kudu-scan-node.cc index b247c67..e171afa 100644 --- a/be/src/exec/kudu-scan-node.cc +++ b/be/src/exec/kudu-scan-node.cc @@ -148,6 +148,7 @@ Status KuduScanNode::Open(RuntimeState* state) { } Status KuduScanNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) { + DCHECK(row_batch != NULL); RETURN_IF_ERROR(ExecDebugAction(TExecNodePhase::GETNEXT, state)); RETURN_IF_CANCELLED(state); RETURN_IF_ERROR(QueryMaintenance(state));
