impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4996: Single-threaded KuduScanNode
Date Wed, 08 Mar 2017 20:49:53 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4996: Single-threaded KuduScanNode

Patch Set 1:

Commit Message:

Line 11: same condition as HdfsScanNodeMt: mt_dop is greater than 1.
>= 1?
File be/src/exec/

Line 57:     : ScanNode(pool, tnode, descs),
is this standard indentation?
File be/src/exec/kudu-scan-node-base.h:

Line 79:   /// The Kudu client and table. Scanners share these instances.
how much overhead does not sharing this imply? (how much memory/how many threads sit behind
these structures?)

if it's a lot, we could share this across fragment instances in the query state.

Line 90:   /// The next index in 'scan_tokens_' to be assigned. Protected by lock_.
there is no lock_ here

Line 93:   RuntimeProfile::Counter* kudu_round_trips_;
these are still in KuduScanNode

Line 102:   RuntimeProfile::Counter* kudu_round_trips() const { return kudu_round_trips_;
why is this useful? since it's private, it's only accessible to this class anyway.
File be/src/exec/

Line 60:   if (scanner_ == nullptr) {
why not do this in open?

Line 81:     scanner_.reset();
why reset here?
File be/src/exec/kudu-scan-node-mt.h:

Line 45:   std::unique_ptr<KuduScanner> scanner_;
the separation of scannode and scanner exists today because of the multi-threading in the
scannode. for the mt version, that's not needed anymore.

let's either merge the scanner into this class now, or leave a todo to do that when we get
rid of the non-mt version.

the advantage is that you remove a level of indirection and get a chance to remove other overhead
(example: right now, you create a copy of the conjunct contexts)
File fe/src/main/java/org/apache/impala/planner/

Line 141:     // Determine backend scan node implementation to use. The optimized MT implementation
fix comment
File tests/query_test/

Line 102: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test

Line 118: # Parquet filtering test rlies on a specific mt_dop value, so keep in its own test

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e4593300e376bc508b78acaea64ffdd2c73a67a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-HasComments: Yes

View raw message