Kathy Sun has posted comments on this change. Change subject: System Database (preview for frontend) ......................................................................
Patch Set 13: (15 comments) 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. Done 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 onl Done Line 84: MetricGroup* all_metrics = ExecEnv::GetInstance()->metrics(); > I think eventually we'll want to factor out the metrics-specific code into Done PS13, Line 119: i > This should be slot_desc->col_pos() here and some places below right? Other Yeah, you are right... fixed it. I can't believe I didn't notice that in so many manually tests :O 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. Done Line 83: // DB that contains all builtins > Comment needs updating. Done PS13, Line 110: getInformationSchemaDb > This needs updatnig. Done 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"? Done 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 Done Line 35: public class SystemTable extends Table { > Can maybe it 'final' Does this necessary? Maybe we want to have many types of systemTable at later time? And what about SystemDB class, do we want it be a final class too? PS13, Line 36: TransferMap > Maybe SYSTEM_TABLE_NAME_MAP? Done PS13, Line 37: 6 > Don't need to hardcode this size Done Line 55: // type > Strange comment? At first we thought maybe it's better to all the enum as InfoSchemaTableType not Name. I thought we may discuss which is better. Line 83: // do nothing since isLoaded already > Maybe comment should be: Done 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 133: // TODO Auto-generated method stub > Old comment. Done -- 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
