----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32406/#review78298 -----------------------------------------------------------
Overall looks good. Few minor code level comments. common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/32406/#comment126836> nit : whitespace ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Attr.java <https://reviews.apache.org/r/32406/#comment126837> Can be overridden from o.a.h.h.common.ObjectPair with overriding equals? Also, consider placing this class in o.a.h.h.common package since ql/ package is distributed to cluster and we want to minimize its size. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Op.java <https://reviews.apache.org/r/32406/#comment126838> will be good to add comment for this. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Op.java <https://reviews.apache.org/r/32406/#comment126839> Better to name it: connections. We may use it for other purposes later. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Stage.java <https://reviews.apache.org/r/32406/#comment126962> Add comments for this boolean. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Stage.java <https://reviews.apache.org/r/32406/#comment126966> Seems like this boolean is used to keep state of printer while visiting over plan. If so, better design IMO is to keep this state with printer. This class should be state free. Just a thought, you may know better. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Vertex.java <https://reviews.apache.org/r/32406/#comment126967> Can you add comments about this and next boolean. ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Vertex.java <https://reviews.apache.org/r/32406/#comment126968> Similar comment about state being with printer. - Ashutosh Chauhan On March 26, 2015, 8:10 p.m., pengcheng xiong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32406/ > ----------------------------------------------------------- > > (Updated March 26, 2015, 8:10 p.m.) > > > Review request for hive, Ashutosh Chauhan and John Pullokkaran. > > > Repository: hive-git > > > Description > ------- > > Current Hive Explain (default) is targeted at MR Audience. We need a new > level of explain plan to be targeted at RDBMS audience. The explain requires > these: > 1) The focus needs to be on what part of the query is being executed rather > than internals of the engines > 2) There needs to be a clearly readable tree of operations > 3) Examples - Table scan should mention the table being scanned, the Sarg, > the size of table and expected cardinality after the Sarg'ed read. The join > should mention the table being joined with and the join condition. The > aggregate should mention the columns in the group-by. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java cf82e8b > itests/src/test/resources/testconfiguration.properties 288270e > ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java 149f911 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Attr.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Connection.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Op.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Stage.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/TezJsonParser.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/explain/Vertex.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/merge/MergeFileWork.java e572338 > ql/src/java/org/apache/hadoop/hive/ql/io/rcfile/stats/PartialScanWork.java > 095afd4 > > ql/src/java/org/apache/hadoop/hive/ql/io/rcfile/truncate/ColumnTruncateWork.java > 092f627 > > ql/src/java/org/apache/hadoop/hive/ql/parse/AlterTablePartMergeFilesDesc.java > eaf3dc4 > ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java > 38b6d96 > ql/src/java/org/apache/hadoop/hive/ql/plan/AbstractOperatorDesc.java > 476dfd1 > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterDatabaseDesc.java e45bc26 > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterIndexDesc.java db2cf7f > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableDesc.java 24cf1da > ql/src/java/org/apache/hadoop/hive/ql/plan/ArchiveWork.java 9fb5c8b > ql/src/java/org/apache/hadoop/hive/ql/plan/BaseWork.java 1737a34 > ql/src/java/org/apache/hadoop/hive/ql/plan/BucketMapJoinContext.java > f436bc0 > ql/src/java/org/apache/hadoop/hive/ql/plan/CollectDesc.java 588e14d > ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsDesc.java a44c8e8 > ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java > d644155 > ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsWork.java 3cae727 > ql/src/java/org/apache/hadoop/hive/ql/plan/CommonMergeJoinDesc.java 2354139 > ql/src/java/org/apache/hadoop/hive/ql/plan/CopyWork.java 3353384 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateDatabaseDesc.java a6b52aa > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateFunctionDesc.java dce5ece > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateMacroDesc.java 3c5a723 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java 8cadb96 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableLikeDesc.java 3dad4ab > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateViewDesc.java dd76a82 > ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java 79d9d16 > ql/src/java/org/apache/hadoop/hive/ql/plan/DemuxDesc.java 62de2e4 > ql/src/java/org/apache/hadoop/hive/ql/plan/DependencyCollectionWork.java > 35180cd > ql/src/java/org/apache/hadoop/hive/ql/plan/DescDatabaseDesc.java 3c0ed2a > ql/src/java/org/apache/hadoop/hive/ql/plan/DescFunctionDesc.java 814ad73 > ql/src/java/org/apache/hadoop/hive/ql/plan/DescTableDesc.java eefd4d4 > ql/src/java/org/apache/hadoop/hive/ql/plan/DropDatabaseDesc.java 66d8768 > ql/src/java/org/apache/hadoop/hive/ql/plan/DropFunctionDesc.java e1f93a1 > ql/src/java/org/apache/hadoop/hive/ql/plan/DropMacroDesc.java 3e2aefc > ql/src/java/org/apache/hadoop/hive/ql/plan/DropTableDesc.java c79710d > ql/src/java/org/apache/hadoop/hive/ql/plan/DummyStoreDesc.java 04e556d > ql/src/java/org/apache/hadoop/hive/ql/plan/DynamicPruningEventDesc.java > d6617b5 > ql/src/java/org/apache/hadoop/hive/ql/plan/Explain.java a3408a0 > ql/src/java/org/apache/hadoop/hive/ql/plan/ExplainWork.java f258d51 > ql/src/java/org/apache/hadoop/hive/ql/plan/ExplosionDesc.java dc56ccd > ql/src/java/org/apache/hadoop/hive/ql/plan/FetchWork.java ef5a655 > ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java 83ebfa3 > ql/src/java/org/apache/hadoop/hive/ql/plan/FilterDesc.java 22fd29e > ql/src/java/org/apache/hadoop/hive/ql/plan/ForwardDesc.java b03fc06 > ql/src/java/org/apache/hadoop/hive/ql/plan/GrantDesc.java f1cb323 > ql/src/java/org/apache/hadoop/hive/ql/plan/GrantRevokeRoleDDL.java 65db04e > ql/src/java/org/apache/hadoop/hive/ql/plan/GroupByDesc.java d6aad9f > ql/src/java/org/apache/hadoop/hive/ql/plan/HashTableDummyDesc.java f15ce48 > ql/src/java/org/apache/hadoop/hive/ql/plan/HashTableSinkDesc.java 03ef704 > ql/src/java/org/apache/hadoop/hive/ql/plan/JoinCondDesc.java 0c65196 > ql/src/java/org/apache/hadoop/hive/ql/plan/JoinDesc.java 990608a > ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewForwardDesc.java > e944b2e > ql/src/java/org/apache/hadoop/hive/ql/plan/LateralViewJoinDesc.java 4c0c978 > ql/src/java/org/apache/hadoop/hive/ql/plan/LimitDesc.java be6d194 > ql/src/java/org/apache/hadoop/hive/ql/plan/LoadDesc.java 68e2afc > ql/src/java/org/apache/hadoop/hive/ql/plan/LoadTableDesc.java f514857 > ql/src/java/org/apache/hadoop/hive/ql/plan/LockDatabaseDesc.java cb66d54 > ql/src/java/org/apache/hadoop/hive/ql/plan/LockTableDesc.java c3c4ba4 > ql/src/java/org/apache/hadoop/hive/ql/plan/MapJoinDesc.java 4ccbef7 > ql/src/java/org/apache/hadoop/hive/ql/plan/MapWork.java f6616fb > ql/src/java/org/apache/hadoop/hive/ql/plan/MapredLocalWork.java 316d306 > ql/src/java/org/apache/hadoop/hive/ql/plan/MapredWork.java f3203bf > ql/src/java/org/apache/hadoop/hive/ql/plan/MergeJoinWork.java b2369fa > ql/src/java/org/apache/hadoop/hive/ql/plan/MoveWork.java e43156f > ql/src/java/org/apache/hadoop/hive/ql/plan/MuxDesc.java 1c75e5e > ql/src/java/org/apache/hadoop/hive/ql/plan/OrcFileMergeDesc.java 7d0ab0c > ql/src/java/org/apache/hadoop/hive/ql/plan/PTFDesc.java 5e63f2f > ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 503117d > ql/src/java/org/apache/hadoop/hive/ql/plan/PrincipalDesc.java 818a8e3 > ql/src/java/org/apache/hadoop/hive/ql/plan/PrivilegeDesc.java 03b3b6b > ql/src/java/org/apache/hadoop/hive/ql/plan/PrivilegeObjectDesc.java 5265289 > ql/src/java/org/apache/hadoop/hive/ql/plan/RCFileMergeDesc.java 476aa46 > ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceSinkDesc.java 28cb3ba > ql/src/java/org/apache/hadoop/hive/ql/plan/ReduceWork.java c78184b > ql/src/java/org/apache/hadoop/hive/ql/plan/RevokeDesc.java c0b74ff > ql/src/java/org/apache/hadoop/hive/ql/plan/RoleDDLDesc.java 2aae751 > ql/src/java/org/apache/hadoop/hive/ql/plan/SMBJoinDesc.java a09fc69 > ql/src/java/org/apache/hadoop/hive/ql/plan/ScriptDesc.java 4f7c0da > ql/src/java/org/apache/hadoop/hive/ql/plan/SelectDesc.java cfcfe17 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowColumnsDesc.java 28d16a3 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowConfDesc.java df385a2 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowCreateTableDesc.java 71520e8 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowDatabasesDesc.java 0ad0658 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowFunctionsDesc.java 5d4a821 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowGrantDesc.java d27da3d > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowIndexesDesc.java 10df6c8 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowLocksDesc.java 1902d36 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowPartitionsDesc.java 4059b92 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowTableStatusDesc.java 15613ed > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowTablesDesc.java 850e964 > ql/src/java/org/apache/hadoop/hive/ql/plan/ShowTblPropertiesDesc.java > 13de46e > ql/src/java/org/apache/hadoop/hive/ql/plan/Statistics.java b0caf23 > ql/src/java/org/apache/hadoop/hive/ql/plan/StatsNoJobWork.java 7e3f0bf > ql/src/java/org/apache/hadoop/hive/ql/plan/StatsWork.java 3cf0f7f > ql/src/java/org/apache/hadoop/hive/ql/plan/SwitchDatabaseDesc.java 0cad7c1 > ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java 0e34aee > ql/src/java/org/apache/hadoop/hive/ql/plan/TableScanDesc.java 6530c37 > ql/src/java/org/apache/hadoop/hive/ql/plan/TezWork.java a03e373 > ql/src/java/org/apache/hadoop/hive/ql/plan/TruncateTableDesc.java 24f453f > ql/src/java/org/apache/hadoop/hive/ql/plan/UDTFDesc.java 741a0e0 > ql/src/java/org/apache/hadoop/hive/ql/plan/UnionDesc.java ec01d74 > ql/src/java/org/apache/hadoop/hive/ql/plan/UnionWork.java 5ef0e07 > ql/src/java/org/apache/hadoop/hive/ql/plan/UnlockDatabaseDesc.java 5c21aa9 > ql/src/java/org/apache/hadoop/hive/ql/plan/UnlockTableDesc.java 62ad027 > ql/src/java/org/apache/hadoop/hive/ql/plan/ptf/PTFExpressionDef.java > a0370bf > ql/src/java/org/apache/hadoop/hive/ql/plan/ptf/PTFQueryInputDef.java > 227b117 > > ql/src/java/org/apache/hadoop/hive/ql/plan/ptf/PartitionedTableFunctionDef.java > d1ad20a > > ql/src/test/org/apache/hadoop/hive/ql/parse/TestUpdateDeleteSemanticAnalyzer.java > 7138d51 > ql/src/test/queries/clientpositive/explainuser_1.q PRE-CREATION > ql/src/test/queries/clientpositive/explainuser_2.q PRE-CREATION > ql/src/test/queries/clientpositive/explainuser_3.q PRE-CREATION > ql/src/test/results/clientpositive/tez/explainuser_1.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainuser_2.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainuser_3.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/32406/diff/ > > > Testing > ------- > > > Thanks, > > pengcheng xiong > >
