Tim Armstrong has posted comments on this change. Change subject: System Database (preview for frontend) ......................................................................
Patch Set 13: (17 comments) I took another look over this, looks like we're making progress. I know you have other work on this patch in progress so feel free to address these comments after that work. http://gerrit.cloudera.org:8080/#/c/3863/13//COMMIT_MSG Commit Message: Line 7: System Database (preview for frontend) I created IMPALA-4024 so we have a JIRA for this. http://gerrit.cloudera.org:8080/#/c/3863/13/be/src/exec/system-table-scan-node.cc File be/src/exec/system-table-scan-node.cc: PS13, Line 43: SystemDbScanNode Should be SystemTableScanNode I think. Line 53: table_name_ = tnode.system_table_scan_node.table_name; Can you add a DCHECK here that it's the metrics table (since that's the only one we support). Line 84: MetricGroup* all_metrics = ExecEnv::GetInstance()->metrics(); I think eventually we'll want to factor out the metrics-specific code into a separate class. E.g. MetricsScanner. The current approach works while we're building this up. Let's add a TODO (maybe in the .h file) and consider whether we should do that in the initial patch or not. I think it will be good to do since we can define a clearer interface for adding other system tables. PS13, Line 119: i This should be slot_desc->col_pos() here and some places below right? Otherwise it won't work if we select, say, the second column in the table only. http://gerrit.cloudera.org:8080/#/c/3863/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS13, Line 66: system_database Maybe just "system"? I think other databases call it this. Line 83: // DB that contains all builtins Comment needs updating. PS13, Line 110: getInformationSchemaDb This needs updatnig. http://gerrit.cloudera.org:8080/#/c/3863/13/fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java File fe/src/main/java/com/cloudera/impala/catalog/SystemDb.java: Line 32: "System database, including impala metrics currently"; Maybe something like "System database for Impala cluster"? http://gerrit.cloudera.org:8080/#/c/3863/13/fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java File fe/src/main/java/com/cloudera/impala/catalog/SystemTable.java: Line 1: // Copyright 2016 Cloudera Inc. We should switch all the headers to the new ASF ones Line 35: public class SystemTable extends Table { Can maybe it 'final' PS13, Line 36: TransferMap Maybe SYSTEM_TABLE_NAME_MAP? PS13, Line 37: 6 Don't need to hardcode this size Line 55: // type Strange comment? Line 83: // do nothing since isLoaded already Maybe comment should be: // Table is always loaded. http://gerrit.cloudera.org:8080/#/c/3863/13/fe/src/main/java/com/cloudera/impala/planner/SystemTableScanNode.java File fe/src/main/java/com/cloudera/impala/planner/SystemTableScanNode.java: Line 78: private void computeAllScanRangeLocations(Analyzer analyzer) { This general approach looks reasonable. Line 133: // TODO Auto-generated method stub Old comment. -- To view, visit http://gerrit.cloudera.org:8080/3863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd Gerrit-PatchSet: 13 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun <[email protected]> Gerrit-Reviewer: Kathy Sun <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
