Tim Armstrong has posted comments on this change. Change subject: draft version -- not completed yet ......................................................................
Patch Set 1: (20 comments) Looks like you're making good progress. I tried to compile and play around but ran into some compile errors. I noticed a couple of other things in the code: in AlterTableStmt there is some code that prevents altering DataSource tables. I think we want the same thing for InformationSchema. I think we discussed your two points face-to-face but let me know if you want to chat more. http://gerrit.cloudera.org:8080/#/c/3863/1/be/src/exec/info-schema-scan-node.cc File be/src/exec/info-schema-scan-node.cc: Line 61: ExecEnv::GetInstance()->metrics()->GetChildGroup("impala_metrics")->ToJson( I think before we merge this code we'll want to change this so that it gets the metric values directly rather than converting them to JSON. I think maybe it would make the most sense to define a thrift structure TMetric and add methods to the Metrics class to return those as well as Json. You could return the name, description, and value (the last maybe using the TColumnValue structure) Line 122: // didn't obey 1024-row batch rule, assume batch always < 1024 row_batch->AtCapacity() checks to see if the batch is full, so you are ok here. You are checking it after you add each row. Line 163: if (is_closed()) This can be one line. Line 167: PeriodicCounterUpdater::StopTimeSeriesCounter( This can be one line. Line 174: // TODO need to discuss what should be print here These DebugString() methods are mainly to help with debugging. I don't think the define it for many ExecNode subclasses, so you can just delete this function if you li.e http://gerrit.cloudera.org:8080/#/c/3863/1/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: Line 370: std::string table_name_; We'll also want TInfoSchemaTableType here too http://gerrit.cloudera.org:8080/#/c/3863/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 353: // // Network address of a master host in the form of 0.0.0.0:port I think we'll also need TInfoSchemaTableName here so that we know which table this is. Maybe we should rename it TInfoSchemaTableType PS1, Line 408: Set Extra word. http://gerrit.cloudera.org:8080/#/c/3863/1/common/thrift/PlanNodes.thrift File common/thrift/PlanNodes.thrift: Line 21: We should avoid adding extra whitespace like this. Line 257: Extra line PS1, Line 260: impala_metrics We should use upper case for info schema table names. http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS1, Line 63: _impala_information_schema I think we should just call it "information_schema". http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaDb.java File fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaDb.java: Line 1: package com.cloudera.impala.catalog; Make sure to include the ASF license headers at the top of all new files (it's ok if you didn't get around to this yet). PS1, Line 11: l,null Will need to clean up the formatting here. http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaTable.java File fe/src/main/java/com/cloudera/impala/catalog/InformationSchemaTable.java: Line 35: this.addColumn(new Column("Name", Type.STRING, 0)); We'll need to make this depend on the type of the information schema table. I think we also should make the column names lower-case (I.e. name, value, description) since that's how all the names are stored internally in the Impala catalog. http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/planner/InfoSchemaScanNode.java File fe/src/main/java/com/cloudera/impala/planner/InfoSchemaScanNode.java: Line 1: // Copyright 2016 Cloudera Inc. You'll need to replace all these Cloudera headers with ASF headers. E.g. see the ones in https://gerrit.cloudera.org/#/c/3779/ Line 41: private final static Logger LOG = LoggerFactory Need to fix up these tabs. PS1, Line 99: eed to change to metrics.size() or estimate Good idea. I think it's probably metrics.size() * number of backends? http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java File fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java: Line 1328: } else { There are some tabs in here - maybe you need to configure your editor to always replace tabs with spaces? http://gerrit.cloudera.org:8080/#/c/3863/1/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: Line 598: public List<String> getTableNames(String dbName, String tablePattern, User user) I think you need to rebase onto the latest master, it looks like there someone else's changes are mixed in with yours. If you accidentally committed these changes, you can revert them bit-by-bit, e.g. using git checkout -p HEAD^ path/to/the/file.java -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun <kathy....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
