You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2016/10/11 00:08:43 UTC
[Impala-ASF-CR] IMPALA-1473: Incorrect cardinality in exec summary for exchange
Thomas Tauber-Marshall has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4679
Change subject: IMPALA-1473: Incorrect cardinality in exec summary for exchange
......................................................................
IMPALA-1473: Incorrect cardinality in exec summary for exchange
When there is a merging exchange with a limit, we may copy rows into
the output batch beyond the limit. In this case, we currently update
the output batch's size to reflect the limit, but we also need to
update ExecNode::num_rows_returned_ or the exec summary may show
that the exchange node returned more rows than it really did.
Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
---
M be/src/exec/exchange-node.cc
A tests/query_test/test_exec_summary.py
2 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/4679/1
--
To view, visit http://gerrit.cloudera.org:8080/4679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4679/2//COMMIT_MSG
Commit Message:
PS2, Line 7: IMPA
> nit: can you separate this into separate IMPALA-1473, sometimes ppl are gre
Done
http://gerrit.cloudera.org:8080/#/c/4679/2/shell/impala_client.py
File shell/impala_client.py:
PS2, Line 145: # is the max over all
> I think you need to update impala_beeswax.py which duplicates this code (un
Done
http://gerrit.cloudera.org:8080/#/c/4679/2/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:
PS2, Line 25: test_merge_exchange_num_rows(s
> test_merge_exchange_num_rows
Done
PS2, Line 26: IMPALA-1473
> I think this tests both 1473 and IMPALA-3002. Can you verify that with a pr
It does actually exercise the relevant code for IMPALA-3002 (in impala_beeswax at least), but the result is the same with or without the fix for this particular query, so I added another test.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1473: Incorrect cardinality in exec summary for exchange
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1473: Incorrect cardinality in exec summary for exchange
......................................................................
Patch Set 1:
> (3 comments)
>
> Thanks!
>
> While this fixes the summary, I'm still seeing the wrong result in
> the profile's coordinator RowsProduced. (I had mentioned that in
> the JIRA summary). We should fix up these things at the same time
> since it's confusing if the info doesn't make sense.
>
> When I ran the query in the test I found:
> Coordinator Fragment F02:(Total: 271.450ms, non-child: 2.134ms, %
> non-child: 0.79%)
> MemoryUsage(500.000ms): 16.06 KB, 20.08 KB
> - AverageThreadTokens: 0.00
> - BloomFilterBytes: 0
> - PeakMemoryUsage: 20.08 KB (20560)
> - PerHostPeakMemUsage: 0
> - PrepareTime: 121.367us
> - RowsProduced: 0 (0)
>
>
> RowsProduced should be 5. Not sure why it's 0, but the
> PlanFragmentExecutor::GetNext() calls ExchangeNode::GetNext() and
> then sets
>
> COUNTER_ADD(rows_produced_counter_, row_batch_->num_rows());
>
>
> Which it looks like isn't coming back or isn't being set properly
> by the ExchangeNode for some reason.
Forgot to mention though that you'll probably need to fix IMPALA-3002 at the same time. That seems like the right thing to do anyway- it makes sense to fix all these known broken 'num rows' issues at the same time.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
......................................................................
IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
IMPALA-3002:
The shell prints an incorrect value for '#Rows' in the exec
summary for broadcast nodes due to incorrect logic around
whether to use max or agg stats. This patch makes the behavior
consistent with the way the be treats exec summaries in
summary-util.cc. This incorrect logic was also duplicated in
the impala_beeswax test framework.
IMPALA-1473:
When there is a merging exchange with a limit, we may copy rows
into the output batch beyond the limit. In this case, we currently
update the output batch's size to reflect the limit, but we also
need to update ExecNode::num_rows_returned_ or the exec summary
may show that the exchange node returned more rows than it really
did.
Additionally, PlanFragmentExecutor::GetNext does not update
rows_produced_counter_ in some cases, leading the runtime profile
to display an incorrect value for 'RowsProduced'.
Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
Reviewed-on: http://gerrit.cloudera.org:8080/4679
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/exchange-node.cc
M be/src/runtime/plan-fragment-executor.cc
M shell/impala_client.py
M tests/beeswax/impala_beeswax.py
A tests/query_test/test_observability.py
5 files changed, 63 insertions(+), 7 deletions(-)
Approvals:
Matthew Jacobs: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-3002/1473: Cardinality observability cleanup
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
......................................................................
Patch Set 3: Verified+1
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
......................................................................
Patch Set 3: Code-Review+2
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3002/1473: Cardinality observability cleanup
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).
Change subject: IMPALA-3002/1473: Cardinality observability cleanup
......................................................................
IMPALA-3002/1473: Cardinality observability cleanup
IMPALA-3002:
The shell prints an incorrect value for '#Rows' in the exec
summary for broadcast nodes due to incorrect logic around
whether to use max or agg stats. This patch makes the behavior
consistent with the way the be treats exec summaries in
summary-util.cc
IMPALA-1473:
When there is a merging exchange with a limit, we may copy rows
into the output batch beyond the limit. In this case, we currently
update the output batch's size to reflect the limit, but we also
need to update ExecNode::num_rows_returned_ or the exec summary
may show that the exchange node returned more rows than it really
did.
Additionally, PlanFragmentExecutor::GetNext does not update
rows_produced_counter_ in some cases, leading the runtime profile
to display an incorrect value for 'RowsProduced'.
Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
---
M be/src/exec/exchange-node.cc
M be/src/runtime/plan-fragment-executor.cc
M shell/impala_client.py
A tests/query_test/test_observability.py
4 files changed, 45 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/4679/2
--
To view, visit http://gerrit.cloudera.org:8080/4679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#3).
Change subject: IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
......................................................................
IMPALA-3002/IMPALA-1473: Cardinality observability cleanup
IMPALA-3002:
The shell prints an incorrect value for '#Rows' in the exec
summary for broadcast nodes due to incorrect logic around
whether to use max or agg stats. This patch makes the behavior
consistent with the way the be treats exec summaries in
summary-util.cc. This incorrect logic was also duplicated in
the impala_beeswax test framework.
IMPALA-1473:
When there is a merging exchange with a limit, we may copy rows
into the output batch beyond the limit. In this case, we currently
update the output batch's size to reflect the limit, but we also
need to update ExecNode::num_rows_returned_ or the exec summary
may show that the exchange node returned more rows than it really
did.
Additionally, PlanFragmentExecutor::GetNext does not update
rows_produced_counter_ in some cases, leading the runtime profile
to display an incorrect value for 'RowsProduced'.
Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
---
M be/src/exec/exchange-node.cc
M be/src/runtime/plan-fragment-executor.cc
M shell/impala_client.py
M tests/beeswax/impala_beeswax.py
A tests/query_test/test_observability.py
5 files changed, 63 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/4679/3
--
To view, visit http://gerrit.cloudera.org:8080/4679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I386719370386c9cff09b8b35d15dc712dc6480aa
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-1473: Incorrect cardinality in exec summary for exchange
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-1473: Incorrect cardinality in exec summary for exchange
......................................................................
Patch Set 1:
(3 comments)
Thanks!
While this fixes the summary, I'm still seeing the wrong result in the profile's coordinator RowsProduced. (I had mentioned that in the JIRA summary). We should fix up these things at the same time since it's confusing if the info doesn't make sense.
When I ran the query in the test I found:
Coordinator Fragment F02:(Total: 271.450ms, non-child: 2.134ms, % non-child: 0.79%)
MemoryUsage(500.000ms): 16.06 KB, 20.08 KB
- AverageThreadTokens: 0.00
- BloomFilterBytes: 0
- PeakMemoryUsage: 20.08 KB (20560)
- PerHostPeakMemUsage: 0
- PrepareTime: 121.367us
- RowsProduced: 0 (0)
RowsProduced should be 5. Not sure why it's 0, but the PlanFragmentExecutor::GetNext() calls ExchangeNode::GetNext() and then sets
COUNTER_ADD(rows_produced_counter_, row_batch_->num_rows());
Which it looks like isn't coming back or isn't being set properly by the ExchangeNode for some reason.
http://gerrit.cloudera.org:8080/#/c/4679/1/tests/query_test/test_exec_summary.py
File tests/query_test/test_exec_summary.py:
PS1, Line 27: # Search through the query profile for the exec summary, of the form:
: # Operator #Hosts Avg Time Max Time #Rows Est. #Rows ...
: # ------------------------------------------------------------------- ...
: # 05:MERGING-EXCHANGE 1 82.547us 82.547us 5 5 ...
: # ...
: def _get_exec_summary(self, handle):
: profile = self.client.get_runtime_profile(handle)
: summary = ''
: in_summary = False
: for line in profile.split('\n'):
: if in_summary:
: if line.startswith('-----'):
: summary += line + '\n'
: else:
: split = line.split(':')
: if len(split) != 2 or not split[0].isdigit():
: return summary
: summary += line + '\n'
: elif line.startswith('Operator'):
: # Found the beginning of the exec summary.
: summary += line + '\n'
: in_summary = True
: assert False, 'Failed to find exec summary in query profile'
Thanks for adding a test. However, there's already logic to parse the summary into a structured result, see impala_beeswax.py. Can you change the code to leverage that?
PS1, Line 57: handle = self.execute_query_async(query, dict())
: self.impalad_test_service.wait_for_query_state(self.client, handle,
: self.client.QUERY_STATES['FINISHED'], timeout=40)
: self.client.fetch(query, handle)
: self.close_query(handle)
There are 'execute' versions that handle waiting and closing for you. Take a look in impala_test_suite. You should be able to find a way to call the client's execute method (not the async version) and get back the result object that has the parsed summary in it.
PS1, Line 64: for line in self._get_exec_summary(handle).split('\n'):
: if line.startswith('05:MERGING-EXCHANGE'):
: found = True
: columns = re.split(' +', line)
: assert columns[0] == '05:MERGING-EXCHANGE' # Operator
: assert columns[4] == '5' # #Rows
: assert columns[5] == '5' # Est. #Rows
: break
you should be able to avoid the string parsing yourself and use the structured result from ImpalaBeeswaxClient
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3002/1473: Cardinality observability cleanup
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-3002/1473: Cardinality observability cleanup
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4679/1/tests/query_test/test_exec_summary.py
File tests/query_test/test_exec_summary.py:
PS1, Line 27:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
> Thanks for adding a test. However, there's already logic to parse the summa
Done
PS1, Line 57:
:
:
:
:
> There are 'execute' versions that handle waiting and closing for you. Take
Done
PS1, Line 64:
:
:
:
:
:
:
:
> you should be able to avoid the string parsing yourself and use the structu
Done
--
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes