[Impala-CR](cdh5-trunk) Review Only: Kudu Impala integration

["David Ribeiro Alves (Code Review)" <ge...@cloudera.org>]

David Ribeiro Alves has posted comments on this change.

Change subject: Review Only: Kudu Impala integration
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/1403/1/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:

Line 304:             "value of type: $0", NodeTypeToString(node.node_type)));
> Having clear invariants is important to make our software maintainable and 
The problem is precisely how "clear" the invariants are. If the state space of some part of the code is big enough (like expression pushdowns might eventually be) that you don't test all the possibilities exhaustively (either now or in the future) then you're not sure that the "invariant" is in fact an "invariant".
In any case I was not talking about the double-handling of errors (that  makes sense to me since it effectively already handles the problem) I was referring to the general elision of checks (with no alternative handling) because of frontend enforced invariants that turned out not to be invariants at all, or that eventually stopped being invariants.


http://gerrit.cloudera.org:8080/#/c/1403/1/be/src/exec/kudu-scanner.cc
File be/src/exec/kudu-scanner.cc:

Line 232: idx == RowBatch::INVALID_ROW_INDEX
> Marking this logic change to a follow up patch.
Left a TODO


http://gerrit.cloudera.org:8080/#/c/1403/1/common/thrift/DataSinks.thrift
File common/thrift/DataSinks.thrift:

Line 31:   KUDU_INSERT
> why is that a big change? seems like just one more enum.
Ended up doing this.


http://gerrit.cloudera.org:8080/#/c/1403/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

Line 485: 
> ?
Done


Line 715:   KW_DELETE opt_modify_ignore:ignore opt_delete_from:target_table  where_clause:where
> this is a minimal change, please do it now.
Done


http://gerrit.cloudera.org:8080/#/c/1403/1/fe/src/main/java/com/cloudera/impala/analysis/DistributeComponent.java
File fe/src/main/java/com/cloudera/impala/analysis/DistributeComponent.java:

Line 93:               "of projected key columns: %d != %d", splitRow.size(), columns_.size()));
> i feel like that's a correctness issue, which means it needs to be addresse
adding new tests/functionality and doing big refactors in a merge patch is not ideal  either. Mainly it forces painful iterations on a 8000+ loc patch for a particular issue, which would be much more tractable to address after and likely to better resolved too. Finally tests that cover this exist, so it's not like it doesn't work, it's possible that it has a bug that hasn't been covered.
I filed IMPALA-3156 to address this post-merge


http://gerrit.cloudera.org:8080/#/c/1403/1/fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java
File fe/src/main/java/com/cloudera/impala/catalog/KuduTable.java:

Line 156:       // TODO: Check for decimal types?
> marked where?
created IMPALA-3157


Line 255:         builder.add("-1");
> again, not sure, marked for follow up.
left a TODO


http://gerrit.cloudera.org:8080/#/c/1403/1/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java:

Line 1506:         "if kudu.table_name, kudu.master_addresses, and kudu.key_columns are " +
> i suggest tableParamsAreValid returns a string that if non-empty contains t
created IMPALA-3158 for this


Line 1590:         "range(a) split rows ((1),('abc'),(3)) " +
> good question. doesn't seem like it should be. at the very least it seems l
testing this would be part of  IMPALA-3156


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I239314acbc8434ef673a3a59d2a82a0338ea5876
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: David Ribeiro Alves <da...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Martin Grund <gr...@gmail.com>
Gerrit-Reviewer: Silvius Rus <sr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes