----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22996/#review47170 -----------------------------------------------------------
Hey Jason, looks good! Nice work! I have a question or two below and a bit nits. itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniMr.java <https://reviews.apache.org/r/22996/#comment82798> When the error message does not contain the text we are looking for, putting the actual text in the error message is useful. I.e. when this assertion fails we won't have any idea what the actual message was. Thus the person debugging will have to actually make a code change and re-run the test to see what happened. ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java <https://reviews.apache.org/r/22996/#comment82794> I am sure this is a stupid question but why are we subclassing HMSC? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java <https://reviews.apache.org/r/22996/#comment82782> nit: Is "Partition columns are not supported on temporary tables and source table in CREATE TABLE LIKE is partitioned." more clear? ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/22996/#comment82783> It looks to me like these can be private since they are not accessed outside this class? ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/22996/#comment82780> These // should be javadoc style. ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/22996/#comment82779> I understand it's coded today such that these three conf.get() will not return null. However I believe we should use Preconditions.checkNotNull here to ensure once that assumption is not true we don't give the dev/user a terrible error message. ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/22996/#comment82785> nit: Is "Cannot create directory" more clear? ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java <https://reviews.apache.org/r/22996/#comment82786> Setter is not being used. - Brock Noland On June 28, 2014, 12:35 a.m., Jason Dere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22996/ > ----------------------------------------------------------- > > (Updated June 28, 2014, 12:35 a.m.) > > > Review request for hive, Gunther Hagleitner, Navis Ryu, and Harish Butani. > > > Bugs: HIVE-7090 > https://issues.apache.org/jira/browse/HIVE-7090 > > > Repository: hive-git > > > Description > ------- > > Temp tables managed in memory by SessionState. > SessionHiveMetaStoreClient overrides table-related methods in HiveMetaStore > to access the temp tables saved in the SessionState when appropriate. > > > Diffs > ----- > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniMr.java > 9fb7550 > itests/qtest/testconfiguration.properties 1462ecd > metastore/if/hive_metastore.thrift cc802c6 > metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 9e8d912 > ql/src/java/org/apache/hadoop/hive/ql/Context.java abc4290 > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java d8d900b > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 4d35176 > > ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java 3df2690 > > ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java > 1270520 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g f934ac4 > ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java > 71471f4 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 83d09c0 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableDesc.java 2537b75 > ql/src/java/org/apache/hadoop/hive/ql/plan/CreateTableLikeDesc.java cb5d64c > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 2143d0c > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezTask.java 43125f7 > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager.java 98c3cc3 > ql/src/test/org/apache/hadoop/hive/ql/parse/TestMacroSemanticAnalyzer.java > 91de8da > > ql/src/test/org/apache/hadoop/hive/ql/parse/authorization/TestHiveAuthorizationTaskFactory.java > 20d08b3 > ql/src/test/queries/clientnegative/temp_table_authorize_create_tbl.q > PRE-CREATION > ql/src/test/queries/clientnegative/temp_table_column_stats.q PRE-CREATION > ql/src/test/queries/clientnegative/temp_table_create_like_partitions.q > PRE-CREATION > ql/src/test/queries/clientnegative/temp_table_index.q PRE-CREATION > ql/src/test/queries/clientnegative/temp_table_partitions.q PRE-CREATION > ql/src/test/queries/clientnegative/temp_table_rename.q PRE-CREATION > ql/src/test/queries/clientpositive/show_create_table_temp_table.q > PRE-CREATION > ql/src/test/queries/clientpositive/stats19.q 51514bd > ql/src/test/queries/clientpositive/temp_table.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_external.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_gb1.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_join1.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_names.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_options1.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_precedence.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_subquery1.q PRE-CREATION > ql/src/test/queries/clientpositive/temp_table_windowing_expressions.q > PRE-CREATION > ql/src/test/results/clientnegative/temp_table_authorize_create_tbl.q.out > PRE-CREATION > ql/src/test/results/clientnegative/temp_table_column_stats.q.out > PRE-CREATION > ql/src/test/results/clientnegative/temp_table_create_like_partitions.q.out > PRE-CREATION > ql/src/test/results/clientnegative/temp_table_index.q.out PRE-CREATION > ql/src/test/results/clientnegative/temp_table_partitions.q.out PRE-CREATION > ql/src/test/results/clientnegative/temp_table_rename.q.out PRE-CREATION > ql/src/test/results/clientpositive/nullformat.q.out d311825 > ql/src/test/results/clientpositive/nullformatCTAS.q.out cab23d5 > ql/src/test/results/clientpositive/show_create_table_alter.q.out 206f4f8 > ql/src/test/results/clientpositive/show_create_table_db_table.q.out 528dd36 > ql/src/test/results/clientpositive/show_create_table_delimited.q.out > d4ffd53 > ql/src/test/results/clientpositive/show_create_table_serde.q.out a9e92b4 > ql/src/test/results/clientpositive/show_create_table_temp_table.q.out > PRE-CREATION > ql/src/test/results/clientpositive/temp_table.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_external.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_gb1.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_join1.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_names.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_options1.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_precedence.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_subquery1.q.out PRE-CREATION > ql/src/test/results/clientpositive/temp_table_windowing_expressions.q.out > PRE-CREATION > ql/src/test/results/clientpositive/tez/temp_table.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/22996/diff/ > > > Testing > ------- > > > Thanks, > > Jason Dere > >
