Kathy Sun has posted comments on this change. Change subject: draft version -- not completed yet ......................................................................
Patch Set 2: (19 comments) 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 I'll leave it for a while. maybe after it could run Line 122: int num_ctxs = conjunct_ctxs_.size(); > row_batch->AtCapacity() checks to see if the batch is full, so you are ok h What I mean here is I assume the batch is smaller than 1024 rows, and I could get all of it at one time, instead of get a piece of the whole by getNextInputBatch after using out of it. just simplify question here, I'll expand it after it could run Line 163: void InfoSchemaScanNode::Close(RuntimeState* state) { > This can be one line. Done Line 167: PeriodicCounterUpdater::StopRateCounter(total_throughput_counter()); > This can be one line. Done Line 174: stringstream* out) const { > These DebugString() methods are mainly to help with debugging. I don't thin Done 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 I'm not very sure here. In common/thriftPlanNode.thrift I add a enum, I considered TInfoSchemaTableType, but I thought it could be reasonable to name it as TInfoSchemaTableName, since there is only one table each type. enum TInfoSchemaTableName { impala_metrics, datastream_manager, impala_server, jvm, scheduler, statestore_subscriber, } Did I get you? Or you mean type is not about subgroup in metrics. Like this? enum TInfoSchemaTableType { metrics, catalog, } foreshadow for later expansion? 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 tab If add TInfoSchemaTableName( or Type), it will be the same as table_name, isn't it? so there is redundant. I can replace string table_name with TInfoSchemaTableName...? PS1, Line 408: Set > Extra word. Done 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. Done Line 257: > Extra line Done PS1, Line 260: impala_metrics > We should use upper case for info schema table names. Done 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". Done 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 (i Done PS1, Line 11: > Will need to clean up the formatting here. Done 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: org.apache.hadoop.hive.metastore.api.Table msTable, Db db, String name, > We'll need to make this depend on the type of the information schema table. Done 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. se I'll do it in a single patch later, not to mix those with other changes Line 41: private final static Logger LOG = LoggerFactory > Need to fix up these tabs. fix in next patch as well PS1, Line 99: eed to change to metrics.size() or estimate > Good idea. I think it's probably metrics.size() * number of backends? discuss offline 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 al Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun <kathy....@cloudera.com> Gerrit-Reviewer: Kathy Sun <kathy....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
