Tim Armstrong has posted comments on this change. Change subject: Infomation_schema (preview for frontend) ......................................................................
Patch Set 6: (18 comments) Did a pass over it and made some general comments. I think we should do some cleanup and renaming of things before getting more eyes on it, so that the review will go as smoothly as possible for everyone. http://gerrit.cloudera.org:8080/#/c/3863/6/be/src/exec/info-schema-scan-node.cc File be/src/exec/info-schema-scan-node.cc: PS6, Line 100: ColNames Constants should be named like: COL_NAMES PS6, Line 136: tuple_pool I think we can get rid of the 'tuple_pool' variable and just use row_batch->tuple_data_pool() directly. (this pattern appears in other scanners for historical reasons) Line 153: while (!ReachedLimit() && !row_batch->AtCapacity() This loop looks sufficient to me to materialise any number of rows. Line 168: idx++; Our coding standard says to use '++idx'. Not a big deal but I think it's easiest to fix now. Line 192: // TODO need to discuss what should be print here You can actually remove DebugString(). We don't have it for all scan nodes. E.g. HdfsScanNode doesn't have an implementation. http://gerrit.cloudera.org:8080/#/c/3863/6/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java File fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java: Line 532: case VIEW_METADATA: Can you add a TODO like: // TODO before committing: implement authorization for system tables So that other reviewers know we intend to fix this. Line 533: case SELECT: There are some tabs instead of spaces here. Let's just figure out how to set up your text editor to use spaces instead of tabs, it will save time overall. http://gerrit.cloudera.org:8080/#/c/3863/6/fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaTable.java File fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaTable.java: PS6, Line 34: InformationSchemaTable I spent some time thinking about this and I'm thinking we should call this SystemTable/SystemDatabase. Since information_schema is a special kind of system database that shows catalog metadata, and probably shouldn't include other non-catalog metadata. Line 45: Your text editor is leaving in trailing whitespace - it should be possible to configure it to avoid this (it will save time in the long run). PS6, Line 52: Name We should keep the column names lower-case (they're converted to lower-case anyway). Line 89: // do nothing since isLoaded already Is this ever called? I think since isLoaded() is always true, this should never be called. If that's correct, we should just have this be: Preconditions.checkState(false); To assert that load() is never called for this table type. Line 109: // TODO Auto-generated method stub Comment not needed. http://gerrit.cloudera.org:8080/#/c/3863/6/fe/src/main/java/com/cloudera/impala/planner/InfoSchemaScanNode.java File fe/src/main/java/com/cloudera/impala/planner/InfoSchemaScanNode.java: Let's clean up the formatting in this file before getting others to review it - it's hard to read in gerrit. PS6, Line 42: InfoSchemaScanNode Maybe this should be SystemTableScanNode (assuming we do the rename I suggested). PS6, Line 45: KuduScanNode Needs updating Line 71: // TODO: Does the port matter? Add a TODO to have one scan range per backend (so other reviewers can see what we are planning). http://gerrit.cloudera.org:8080/#/c/3863/6/fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java File fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java: PS6, Line 1256: tblRef.getTable() Let's do some cleanup here by changing this to 'table'. I was confused why it was different. http://gerrit.cloudera.org:8080/#/c/3863/6/testdata/workloads/functional-planner/queries/PlannerTest/info-schema.test File testdata/workloads/functional-planner/queries/PlannerTest/info-schema.test: When I open this file locally there are a bunch of nonsense characters. I think that's why it thinks this is a binary file. 00:SCAN INFORMATION_SCHEMA [information_schema.impala_metrics]^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@ -- 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: 6 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
