You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2017/02/14 16:35:24 UTC

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans. 

Alex Behm has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6000

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................

IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
13 files changed, 404 insertions(+), 320 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/6000/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6000
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6000/3/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

Line 211:   // have typically gone through several evaluations.
i didn't get the second sentence.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#3).

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................

IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
15 files changed, 292 insertions(+), 215 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/6000/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6000
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 1:

(3 comments)

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

Line 243: Status HdfsTextScanner::FinishScanRange(RowBatch* row_batch) {
> is there some better way to arrange the functions in this file to facilitat
I re-arranged the functions based on their original order. I added the new functions after the bulk of changed existing functions.


http://gerrit.cloudera.org:8080/#/c/6000/1/be/src/exec/hdfs-text-scanner.h
File be/src/exec/hdfs-text-scanner.h:

Line 33: enum TextScanState {
> move into hdfstextscanner, or is this needed outside?
Done


Line 189:   int WriteFields(MemPool* pool, TupleRow* row, int num_fields, int num_tuples);
> output or in/out params go at end
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 2:

(12 comments)

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

Line 64:       batch_start_ptr_(NULL),
use nullptr throughout


Line 273:   MemPool* pool = row_batch->tuple_data_pool();
please consider getting rid of this variable and referring to the rowbatch pool directly. there are too many mempools flying around in this code, it's nice to have a reminder which one is being used.


Line 440:       eos_ = true;
advance scan_state_?

also, i don't see anything in hdfs-scanner.h that says you can't call getnext() when eos() returns true.


Line 445:         << "FE should have generated SNAPPY_BLOCKED instead.";
why not check this when creating the stream?


Line 446:     RETURN_IF_ERROR(UpdateDecompressor(stream_->file_desc()->file_compression));
why is this needed, given that we just started (ie, why not move it into initialization)?


Line 459:     eos_ = true;
scan_state_ transition?


Line 469: Status HdfsTextScanner::CommitRows(RowBatch* row_batch, int num_rows) {
> This loosely corresponds to the previous HdfsScanNode::CommitRows()
how about coalescing that with hdfsscannode::commitrows? at the very least move this into hdfsscannode.


Line 485:     ExprContext::FreeLocalAllocations(entry.second);
why call this here?


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

Line 119:   /// regardless if whether it passed the conjuncts.
regardless of whether


Line 124:   /// Only valid to call in scan state FIRST_TUPLE_FOUND or PAST_SCAN_RANGE.
thanks for introducing that state variable, it's much clearer than a bunch of bools that get passed around.


Line 135:   Status CommitRows(RowBatch* row_batch, int num_rows);
in/out params go last


Line 140:   /// otherwise it will just read num_bytes. If we are reading compressed text, num_bytes
if we are reading compressed text, should this even be called (or why is there a 'compressed stream' variant)?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 5: Code-Review+2

Cluster perf tests indicated no regression in cycles spent in the scan. Still following up with more LZO experiments, but going to merge this change as agreed with Marcel and Mostafa.

This is a coordinated merge with Impala-Lzo, so it won't go through the regular GVO. I manually validated this change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 2:

(2 comments)

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

Line 429: Status HdfsTextScanner::GetNextInternal(RowBatch* row_batch) {
This loosely corresponds to the previous ProcessSplit()


Line 469: Status HdfsTextScanner::CommitRows(RowBatch* row_batch, int num_rows) {
This loosely corresponds to the previous HdfsScanNode::CommitRows()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6000

to look at the new patch set (#5).

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................

IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
15 files changed, 293 insertions(+), 216 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/6000/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6000
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6000/3/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

Line 211:   // have typically gone through several evaluations.
> i didn't get the second sentence.
Rehprased. Can you take another look? Might be that I'm missing something.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has submitted this change and it was merged.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Reviewed-on: http://gerrit.cloudera.org:8080/6000
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Alex Behm <al...@cloudera.com>
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
15 files changed, 293 insertions(+), 216 deletions(-)

Approvals:
  Alex Behm: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 1:

(3 comments)

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

Line 243: Status HdfsTextScanner::FinishScanRange(RowBatch* row_batch) {
is there some better way to arrange the functions in this file to facilitate the comparison? manually piecing together the diff is a bit hard.


http://gerrit.cloudera.org:8080/#/c/6000/1/be/src/exec/hdfs-text-scanner.h
File be/src/exec/hdfs-text-scanner.h:

Line 33: enum TextScanState {
move into hdfstextscanner, or is this needed outside?


Line 189:   int WriteFields(MemPool* pool, TupleRow* row, int num_fields, int num_tuples);
output or in/out params go at end


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#2).

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................

IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
13 files changed, 262 insertions(+), 178 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/6000/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6000
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6000

to look at the new patch set (#4).

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................

IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Implements HdfsLzoTextTextScanner::GetNext() and changes
ProcessSplit() to repeatedly call GetNext() to share the core
scanning code between the legacy ProcessSplit() interface
(ProcessSpit()) and the new GetNext() interface.

These changes were tricky:
- The scanner used to rely on the ability to attach a batch
  to the row-batch queue for freeing resources
- This patch attempts to preserve the resource-freeing behavior
  by clearing resources as soon as they are complete
- In particular, the scanner attempts to skip corrupt/invalid
  data blocks, and we should avoid accumulating memory
  unnecessarily

The other changes are mostly straightforward:
- Add a RowBatch parameter to various functions
- Add a MemPool parameter to various functions for attaching
  memory of completed resources that may still be references
  by returned batches
- Change Close() to free all resources when a nullptr
  RowBatch is passed

Testing:
- Exhaustive tests passed on debug
- Core tests passed on asan
- TODO: Perf testing on cluster

Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
15 files changed, 292 insertions(+), 215 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/6000/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6000
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>

[Impala-ASF-CR] IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-3905: Implements HdfsScanner::GetNext() for text scans.
......................................................................


Patch Set 2:

(12 comments)

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

Line 64:       batch_start_ptr_(NULL),
> use nullptr throughout
Done


Line 273:   MemPool* pool = row_batch->tuple_data_pool();
> please consider getting rid of this variable and referring to the rowbatch 
Done


Line 440:       eos_ = true;
> advance scan_state_?
Done. I think it makes sense to not allow GetNext() once eos() is true because there does not
seem to be a real use case for allowing that, and disallowing it helps simplify the logic in GetNext().
Imo, we should have that contract for ExecNode::GetNext() as well, but that's another story.


Line 445:         << "FE should have generated SNAPPY_BLOCKED instead.";
> why not check this when creating the stream?
Good idea. Done.


Line 446:     RETURN_IF_ERROR(UpdateDecompressor(stream_->file_desc()->file_compression));
> why is this needed, given that we just started (ie, why not move it into in
Moved into InitNewRange()


Line 459:     eos_ = true;
> scan_state_ transition?
Done


Line 469: Status HdfsTextScanner::CommitRows(RowBatch* row_batch, int num_rows) {
> how about coalescing that with hdfsscannode::commitrows? at the very least 
The main problem is that the HdfsScanner::CommitRows() may add the batch to the row-batch queue and allocate a new batch. But for the new scanners we want to manually add batches to the queue after getting another batch with GetNext().

I merged this CommitRows() with HdfsScanner::CommitRows() and added a flag to control the queue-adding behavior. It's slightly awkward, let me know if you have a better idea.


Line 485:     ExprContext::FreeLocalAllocations(entry.second);
> why call this here?
Added a comment. Did you have place in mind? We can probably move it somewhere else, but not sure what side-effects it may have.


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

Line 119:   /// regardless if whether it passed the conjuncts.
> regardless of whether
Done


Line 124:   /// Only valid to call in scan state FIRST_TUPLE_FOUND or PAST_SCAN_RANGE.
> thanks for introducing that state variable, it's much clearer than a bunch 
Thanks!


Line 135:   Status CommitRows(RowBatch* row_batch, int num_rows);
> in/out params go last
Done


Line 140:   /// otherwise it will just read num_bytes. If we are reading compressed text, num_bytes
> if we are reading compressed text, should this even be called (or why is th
Added comment.

This functions internally handles uncompressed text, streaming compressed text (e.g., gzip), and non-streaming compressed text. See FillByteBufferCompressedFile() and FillByteBufferCompressedStream().

In addition, this function is overridden by the LZO scanner to reuse much of the scanning logic in here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id193aa223434d7cc40061a42f81bbb29dcd0404b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes