[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11007 ) Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2875/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/11007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5 Gerrit-Change-Number: 11007 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 30 Jul 2018 06:40:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11007 ) Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS .. Patch Set 11: Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/97/ Running initial code review checks. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/11007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5 Gerrit-Change-Number: 11007 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 30 Jul 2018 06:40:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS
Hello Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11007 to look at the new patch set (#11). Change subject: IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS .. IMPALA-7333: remove MarkNeedsDeepCopy() in agg and BTS This takes advantage of work (e.g. IMPALA-3200, IMPALA-5844) to remove a couple of uses of the API. Testing: Ran core, ASAN and exhaustive builds. Added unit tests to directly test the attaching behaviour. Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5 --- M be/src/exec/grouping-aggregator.cc M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/buffered-tuple-stream.cc M be/src/runtime/buffered-tuple-stream.h M be/src/runtime/row-batch.h 5 files changed, 401 insertions(+), 131 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/11007/11 -- To view, visit http://gerrit.cloudera.org:8080/11007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I91ac53bacc00df4726c015a30ba5a2026aa4b5f5 Gerrit-Change-Number: 11007 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 ) Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements .. Patch Set 11: (1 comment) Can +2 once you fix this remaining bug. http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java: http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@557 PS11, Line 557: for (UnionOperand operand : operands_) { super.collectInlineViews() Add a test for this too. (view self ref from withClause_ in a UnionStmt). -- To view, visit http://gerrit.cloudera.org:8080/10908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7 Gerrit-Change-Number: 10908 Gerrit-PatchSet: 11 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Comment-Date: Mon, 30 Jul 2018 01:21:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3825: Distribute Runtime Filtering Aggregation
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11055 ) Change subject: IMPALA-3825: Distribute Runtime Filtering Aggregation .. Patch Set 3: (40 comments) http://gerrit.cloudera.org:8080/#/c/11055/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11055/3//COMMIT_MSG@14 PS3, Line 14: Add comments about the following, so that it's easier for reviewers: - Explain in a para or two how your patch achieves the distribution; i.e. explain your approach in plain english. - What kind of new failure modes can happen because of this change, as opposed to before? (Talk about the Exec() RPC race with the UpdateFilter() RPC) - How long do we expect the aggregators to be accessible? i.e. what is the lifetime of an aggregator tied to? - How are you updating the runtime profile? http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator-backend-state.cc@95 PS3, Line 95: x naming http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator-backend-state.cc@98 PS3, Line 98: LOG(INFO) << "DebuggingPublishFilter AggregatorAddress " << tfs.aggregator_address; remove http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator-backend-state.cc@95 PS3, Line 95: for (auto const& x : filter_routing_table) { : TFilterState tfs; : x.second.ToThrift(&tfs); : LOG(INFO) << "DebuggingPublishFilter AggregatorAddress " << tfs.aggregator_address; : rpc_params->filter_routing_table.insert( : std::pair(x.first, tfs)); : } Add a comment about what you're doing here. http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@260 PS3, Line 260: x naming. Call it 'filter' or something similar http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@262 PS3, Line 262: num_filters_ Is this needed as a member variable? Doesn't seem to be used anywhere. http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@690 PS3, Line 690: if (params.__isset.filter_updates_received) { Add a comment above this line: "Update aggregator's filter metrics in the runtime profile" http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@691 PS3, Line 691: if (backend_state->GetNumReceivedFilters() < params.filter_updates_received) { Add another comment above this line: "Make sure not to double count filter updates from the same aggregator." http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@690 PS3, Line 690: if (params.__isset.filter_updates_received) { : if (backend_state->GetNumReceivedFilters() < params.filter_updates_received) { : filter_updates_received_->Add( : params.filter_updates_received - backend_state->GetNumReceivedFilters()); : backend_state->SetNumReceivedFilters(params.filter_updates_received); : } : } There's a race here. If 2 updates for the same Backend execute in parallel, you'll end up having an incorrect number of updated filters. http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/coordinator.cc@884 PS3, Line 884: LOG(INFO) : << "DebuggingPublishFilter Coordinator sending filter to fragment with id " : << fragment_idx; remove http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h File be/src/runtime/filter-state.h: http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@141 PS3, Line 141: FilterTarget const ref http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@150 PS3, Line 150: /// Need to cast the int type of this class to int32_t of thrift Still needed? http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@151 PS3, Line 151: set Why not unordered? http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@152 PS3, Line 152: int i int32_t here and remove the cast in the next line. Also, rename to 'idx'. http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@167 PS3, Line 167: TFilterTarget const ref http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@171 PS3, Line 171: boost std http://gerrit.cloudera.org:8080/#/c/11055/3/be/src/runtime/filter-state.h@130 PS3, Line 130: bool disabled() const { : if (is_bloom_filter()) { : return bloom_filter_.always_true; : } else { : DCHECK(is_min_max_filter()); : return min
[Impala-ASF-CR] IMPALA-110 (part 3): Add multiple DISTINCT support to query generator
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/11073 ) Change subject: IMPALA-110 (part 3): Add multiple DISTINCT support to query generator .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11073 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a3f14655719ade7b2f6471c561dba4007fd46fa Gerrit-Change-Number: 11073 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Comment-Date: Sun, 29 Jul 2018 20:48:15 + Gerrit-HasComments: No