impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kathy Sun (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Infomation schema (preview for frontend)
Date Tue, 23 Aug 2016 19:13:47 GMT
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 <kathy.sun@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message