impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3481: Use Kudu ScanToken API for scan ranges
Date Wed, 07 Sep 2016 15:49:11 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges

Patch Set 4:

File be/src/exec/

Line 53: class KuduScanNodeTest : public testing::Test {
we don't have unit tests for any other exec node. why is this an exception?
File be/src/exec/

Line 247:     VLOG(1) << "Thread started: " << name;
1 is too chatty. also, please use the macros.

Line 328:   VLOG(1) << "Thread done: " << name;
File be/src/exec/kudu-scan-node.h:

Line 39: /// are used to retreive the rows for this scan.

Line 120:   void ScannerThread(const string& name, const string* initial_token);
rename to some verb

Line 125:   Status ProcessScanToken(KuduScanner* scanner, const string* scan_token);
why const*? can this be null?
File be/src/exec/

Line 114:       if (batch_done) break;
why break instead of return?
File be/src/exec/kudu-scanner.h:

Line 34: /// Wraps a Kudu client scanner to fetches row batches from Kudu. The Kudu client
to fetch

Line 114:   int cur_kudu_batch_num_scanned_;
File be/src/scheduling/

Line 976:     scan_range_length = 1000;
can't the fe get that out of the scan token?
File fe/src/main/java/com/cloudera/impala/planner/

Line 107:     try (KuduClient client = new KuduClientBuilder(
break before 'new' instead

Line 131:    * TODO: Remove when the columns are fetched from Kudu directly during analysis.
does kudu cache all metadata? where would the column info get fetched from?

Line 139:         tableSchema.getColumn(colName);
note to kudu api designers: it's nice not to use exceptions to signal return values.

is that a sufficient test? what about data types?

Line 165:       // TODO: Consider only using the LEADER replica.
for better cache locality?

Line 167:         TNetworkAddress address = new TNetworkAddress(replica.getRpcHost(),
break before new instead

Line 203:     for (KuduPredicate predicate: kuduPredicates_) {
single line

Line 267:    * bounds of the result can be evaluated with Expr::GetValue(NULL)).
this presumable has some side effects. describe them.

Line 295:     // Cannot push prediates with null literal values
is there a kudu jira for this?

Line 349:   private static ComparisonOp getKuduOperator(BinaryPredicate.Operator op) {
use qualified name for ComparisonOp, easier to identify that it's a "foreign" type that way

Line 351:     case GT: return ComparisonOp.GREATER;
File fe/src/test/java/com/cloudera/impala/planner/

Line 33: public class KuduPlannerTest extends PlannerTestBase {
could we merge this back into the regular planner test? what was the reason for separating

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I160e5849d372755748ff5ba3c90a4651c804b220
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

View raw message