impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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. ( )

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

Patch Set 7:

File be/src/exec/
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.
File be/src/exec/select-node.h:
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:
File be/src/exec/
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!)
PS7, Line 112: 
nit: remove extra vertical whitespace

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Bikramjeet Vig <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Tue, 10 Oct 2017 22:38:56 +0000
Gerrit-HasComments: Yes

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