Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3002/1473: Cardinality observability cleanup ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/4679/2//COMMIT_MSG Commit Message: PS2, Line 7: 1473 nit: can you separate this into separate IMPALA-1473, sometimes ppl are grepping for a full JIRA reference. http://gerrit.cloudera.org:8080/#/c/4679/2/shell/impala_client.py File shell/impala_client.py: PS2, Line 145: if node.is_broadcast: I think you need to update impala_beeswax.py which duplicates this code (unfortunately) as well. Can you add a comment in the fn header that impala_beeswax.py copies this and changes should be made in both places. I actually thought the test case you have would have exercised the fix but if you hadn't changed the impala_beeswax location then maybe not. It may need to be exercised with a different query. http://gerrit.cloudera.org:8080/#/c/4679/2/tests/query_test/test_observability.py File tests/query_test/test_observability.py: PS2, Line 20: TestObservability A bit odd but I don't see a great home for this test in an existing class. Maybe someone else will have a suggestion. PS2, Line 25: test_merge_exchange_with_limit test_merge_exchange_num_rows PS2, Line 26: IMPALA-1473 I think this tests both 1473 and IMPALA-3002. Can you verify that with a print in the impala_beeswax.py code you changed, and update this message if that's the case? -- To view, visit http://gerrit.cloudera.org:8080/4679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
