impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Infomation schema (preview for frontend)
Date Tue, 23 Aug 2016 15:53:41 GMT
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 <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