Alex Behm has posted comments on this change.

Change subject: MPALA-5776: Write partial tuple to the correct mempool
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7639/2/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

Line 269:     bool eosr = true;
Remove. This not needed/used.


Line 299:         boundary_column_.Clear();
> This is subtle because the contents of boundary_column_ are needed until Wr
What's the point of clearing the boundary column here? I don't think the 
scanner is going to touch it after this point.


Line 808:       boundary_row_.Reset();
> It might be clearer if we factored out this reset logic for boundary_ and p
Another idea: Add a separate function for copying the partial tuple into the 
output batch. All this logic can go inside there and we can document/DCHECK 
pre- and post-conditions.


Line 896:     int num_fields, bool copy_strings) {
Can you remove the 'copy_strings' parameter? It's not used in this function and 
not copying seems too complicated to reason about.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovyt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to