Kathy Sun has posted comments on this change. Change subject: Infomation_schema (preview for frontend) ......................................................................
Patch Set 8: (13 comments) Please skip patch 7, it's a wrong reformat. Patch 8 is a new reformat using .clang_format below is the reply I was working on, those will take effect in next patch. 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: alue = m > Constants should be named like: COL_NAMES Done PS6, Line 136: xt** ctxs > I think we can get rid of the 'tuple_pool' variable and just use row_batch- Done Line 153: > This loop looks sufficient to me to materialise any number of rows. unfortunately... I think it's not. I'm still testing this. discuss offline. Line 168: > Our coding standard says to use '++idx'. Not a big deal but I think it's ea Done Line 192 > You can actually remove DebugString(). We don't have it for all scan nodes. nice, that's a relief. :P 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: Done Line 533: case SELECT: > There are some tabs instead of spaces here. Let's just figure out how to se Done 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 to discuss in 1o1 Line 45: > Your text editor is leaving in trailing whitespace - it should be possible Done PS6, Line 52: Name > We should keep the column names lower-case (they're converted to lower-case Done Line 89: /** > Is this ever called? I think since isLoaded() is always true, this should n Done Line 109: TResultSetMetadata resultSchema = new TResultSetMetadata(); > Comment not needed. Done 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: PS6, Line 42: InfoSchemaScanNode > Maybe this should be SystemTableScanNode (assuming we do the rename I sugge discuss offline -- 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: 8 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
