Todd Lipcon has posted comments on this change. Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/4120/4/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 976: scan_range_length = 1000; > can't the fe get that out of the scan token? We still don't have the tablet sizes available to the client yet, it's a TODO item (see jira referenced above) http://gerrit.cloudera.org:8080/#/c/4120/4/fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java File fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java: 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? when you "open" a table, it fetches the metadata (columns, etc) from the master. This isn't a fan-out so it should have low tail latency. On one of our test clusters, 99%ile is 492us, mean 84us. Line 139: tableSchema.getColumn(colName); > note to kudu api designers: it's nice not to use exceptions to signal retur We're more or less following the pattern of JDBC where trying to get an invalid column throws an exception rather than returning null (see https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSetMetaData.java#L487 ) That said, maybe catching IllegalArgumentException would be a better choice? We could also add a new 'bool hasColumn(String name)' if you'd like. Line 295: // Cannot push prediates with null literal values > is there a kudu jira for this? there is now: KUDU-1595 -- To view, visit http://gerrit.cloudera.org:8080/4120 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I160e5849d372755748ff5ba3c90a4651c804b220 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
