Kurt Deschler 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 1: (23 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: FetchLockWaitTimer and ResultFlushTimer and have been added to account > nit: extra "and" after ResultFlushTimer Done http://gerrit.cloudera.org:8080/#/c/20850/1//COMMIT_MSG@15 PS1, Line 15: materializing results outside of the fetch lock. New query option > nit: A line break would be nice somewhere around here. Big wall of text. Done 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: num_rows_to_read, output_expr_evals_, state)); > I think this needs a comment. It's not obvious why this uses num_rows_to_re Done 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::CopyRows(RowBatch* dst, int start_idx, int num_rows) { > nit: can we put this after DeepCopyTo so the ordering is consistent with th Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc@471 PS1, Line 471: DCHECK_LE(num_rows, num_rows_); > This feels a little redundant with the next line. What we actually care abo removed. http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/runtime/row-batch.cc@472 PS1, Line 472: DCHECK_LE(start_idx + num_rows, num_rows_); > Maybe also DCHECK start_idx >= 0, and num_rows > 0? Done 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: MonotonicStopWatch fetch_lock_timer; > nit: No reason this needs to be outside the block where it's used. Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/impala-hs2-server.cc@248 PS1, Line 248: fetch_lock_timer.Start(); > When does this get stopped again? Or used? removed. 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); \ > line too long (108 > 90) Done 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: pendingRowBatch_(), > nit: you don't really need to default initialize all of these. It happens a Now the ctors are non-default. http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@82 PS1, Line 82: DCHECK(!pendingRowBatch_); > nit: could some of this happen in the constructor? You're asserting things Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@90 PS1, Line 90: mem_tracker_.reset(new MemTracker((impala::IntGauge*)nullptr, -1, "Result row batch", state_->query_mem_tracker())); > line too long (120 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@91 PS1, Line 91: expr_mem_tracker_.reset(new MemTracker((impala::IntGauge*)nullptr, -1, "Result expr", mem_tracker_.get())); > line too long (111 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@189 PS1, Line 189: virtual size_t size() override { return row_batch_context_ ? row_batch_context_->size() : num_rows_; } > line too long (104 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@190 PS1, Line 190: virtual Status InitDelayedMaterialization(const RowDescriptor& row_desc, > Please try to match style (spacing, functions descriptions). Please be more specific what you want changed. http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@193 PS1, Line 193: virtual bool DelayedMaterializationEnabled() const override { return delay_materialization_; } > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@198 PS1, Line 198: RETURN_IF_ERROR(AddRows(row_batch_context_->GetExprEvals(), > Re-using AddRows here makes the AddRows conditional feel awkward. How about Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@280 PS1, Line 280: metadata, rowset, stringify_map_keys, expected_result_count, delay_materialization); > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@457 PS1, Line 457: if (!row_batch_context_) { > Should we DCHECK that delay_materialization_ is true? Done http://gerrit.cloudera.org:8080/#/c/20850/1/be/src/service/query-result-set.cc@502 PS1, Line 502: DCHECK(o->row_batch_context_ == nullptr); > Why not DCHECK_EQ? Done http://gerrit.cloudera.org:8080/#/c/20850/1/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/20850/1/tests/query_test/test_observability.py@674 PS1, Line 674: ; > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/20850/1/tests/query_test/test_observability.py@685 PS1, Line 685: ; > flake8: E703 statement ends with a semicolon Done http://gerrit.cloudera.org:8080/#/c/20850/1/tests/query_test/test_observability.py@694 PS1, Line 694: ; > flake8: E703 statement ends with a semicolon Done -- 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: 1 Gerrit-Owner: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Jan 2024 03:28:01 +0000 Gerrit-HasComments: Yes
