impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4236: Codegen CopyRows() for select nodes
Date Tue, 10 Oct 2017 22:38:56 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8196 )

Change subject: IMPALA-4236: Codegen CopyRows() for select nodes
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node-ir.cc
File be/src/exec/select-node-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node-ir.cc@28
PS7, Line 28: limit_
This could return extra rows. E.g. if the limit for the node is 1025, we returned a batch
of 1024 rows and are now processing another input batch of 1024 rows, then this would return
another 1024 rows instead of only 1 more.

It seems like we might be missing some test coverage here of select nodes with limits (it
may also require a single node plan if there's an exchange inserted otherwise).

I think the old ReachedLimit() check plus FOREACH_ROW would work.


http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h
File be/src/exec/select-node.h:

http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@70
PS6, Line 70:   Status CodegenCopyRows(RuntimeState* state);
> Good to know. Apparently missed this change while I was gone.
Yeah, there were actually two LLVM changes that might be of interest to you: https://gerrit.cloudera.org/#/q/status:merged+project:Impala-ASF+branch:master+topic:llvm-upgrade


http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc
File be/src/exec/select-node.cc:

http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc@91
PS7, Line 91:   DCHECK(*eos == false);
This isn't safe - the GetNext() API doesn't require that the caller initializes the output
parameter. (IMO eos should be part of the ExecNode state rather than something transiently
set as an output parameter, but that's a different discussion!)


http://gerrit.cloudera.org:8080/#/c/8196/7/be/src/exec/select-node.cc@112
PS7, Line 112: 
nit: remove extra vertical whitespace



-- 
To view, visit http://gerrit.cloudera.org:8080/8196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0d496d004418468e16b6f564f90f45ebbf87c1e
Gerrit-Change-Number: 8196
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 22:38:56 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message