> On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > Hey Jason, looks good! Nice work! I have a question or two below and a bit > > nits.
Thanks for reviewing! > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestJdbcWithMiniMr.java, > > line 240 > > <https://reviews.apache.org/r/22996/diff/2/?file=619988#file619988line240> > > > > 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. Good, point, will change. > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java, > > line 34 > > <https://reviews.apache.org/r/22996/diff/2/?file=619995#file619995line34> > > > > I am sure this is a stupid question but why are we subclassing HMSC? HiveMetaStoreClient is part of hive-metastore and can only see the classes that are part of hive-metastore or its dependents. So making the changes directly in HiveMetaStoreClient wasn't possible because it does not have visibility to the SessionState which is part of hive-exec. To try to put all of the logic into the HiveMetaStoreClient, I suppose it may have been possible to define an interface for the particular methods we wanted from the SessionState, and try to initialize the HMSC with the SessionState in some way. But given the way the HMSC is used in Hive.createMetaStoreClient it didn't seem like there was a good place to actually call into HMSC to pass in the SessionState. > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java, line > > 10205 > > <https://reviews.apache.org/r/22996/diff/2/?file=620000#file620000line10205> > > > > nit: > > > > Is "Partition columns are not supported on temporary tables and source > > table in CREATE TABLE LIKE is partitioned." more clear? > > Sounds better than my message :) will change. > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 80 > > <https://reviews.apache.org/r/22996/diff/2/?file=620003#file620003line80> > > > > It looks to me like these can be private since they are not accessed > > outside this class? Will change in next patch > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 183 > > <https://reviews.apache.org/r/22996/diff/2/?file=620003#file620003line183> > > > > These // should be javadoc style. will change in next patch > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 383 > > <https://reviews.apache.org/r/22996/diff/2/?file=620003#file620003line383> > > > > 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. will change in next patch > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 445 > > <https://reviews.apache.org/r/22996/diff/2/?file=620003#file620003line445> > > > > nit: > > > > Is "Cannot create directory" more clear? will change in next patch > On July 2, 2014, 2:07 a.m., Brock Noland wrote: > > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 1093 > > <https://reviews.apache.org/r/22996/diff/2/?file=620003#file620003line1093> > > > > Setter is not being used. Ok, will remove - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22996/#review47170 ----------------------------------------------------------- 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 > >
