Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20850 )
Change subject: IMPALA-12533: Support row materialization outside of fetch lock ...................................................................... Patch Set 3: (17 comments) http://gerrit.cloudera.org:8080/#/c/20850/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20850/1//COMMIT_MSG@13 PS1, Line 13: > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1//COMMIT_MSG@15 PS1, Line 15: account for synchrounous time waiting for the fetch lock and cumulative > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/exec/buffered-plan-root-sink.cc@240 PS1, Line 240: DCHECK_GE(num_rows_to_fetch, 0); > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc@469 PS1, Line 469: void RowBatch::DeepCopyTo(RowBatch* dst) { > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc@471 PS1, Line 471: DCHECK_EQ(dst->num_rows_, 0); > removed. Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc@472 PS1, Line 472: DCHECK_GE(dst->capacity_, num_rows_); > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/impala-hs2-server.cc@208 PS1, Line 208: // Release locks before returning to allow result materializaton to be > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/impala-hs2-server.cc@248 PS1, Line 248: RETURN_IF_ERROR( > removed. Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-options.h@53 PS1, Line 53: TImpalaQueryOptions::DELAY_MATERIALIZE_RESULTS_THRESHOLD + 1); \ > Done nit: Previously it looks like we've tried to line up all the '\' on the same column. http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@61 PS1, Line 61: fetch_size_(fetch_size), > Now the ctors are non-default. Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@82 PS1, Line 82: } > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@108 PS1, Line 108: std::vector<ScalarExprEvaluator*> expr_evals_; > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@190 PS1, Line 190: DCHECK_EQ(0, num_rows_); > Please be more specific what you want changed. Functions and variables have blank lines around them, and comments describing their behavior and inputs. That seems to be a consistent stylistic choice in Impala code. http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@198 PS1, Line 198: > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@457 PS1, Line 457: } > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@502 PS1, Line 502: const HS2ColumnarResultSet* o = static_cast<const HS2ColumnarResultSet*>(other); > Done Ack http://gerrit.cloudera.org:8080/#/c/20850/3/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/20850/3/be/src/service/query-result-set.cc@168 PS3, Line 168: Status AddRowsInternal(const vector<ScalarExprEvaluator*>& expr_evals, RowBatch* batch, This should be moved to the 'private' block below. -- To view, visit http://gerrit.cloudera.org:8080/20850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9512aa6022dbcf81e7eb5f559853b89fe80bd23 Gerrit-Change-Number: 20850 Gerrit-PatchSet: 3 Gerrit-Owner: Kurt Deschler <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 10 Jan 2024 21:22:07 +0000 Gerrit-HasComments: Yes
