Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 29, 2014, 6:13 a.m.) Review request for hive and Prasanth_J. Changes --- Addressing review feedback from Prasanth Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs (updated) - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 51c3f2c ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 4cf98d8 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 3f8648b ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 9798cf3 ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java 7cb7c5e ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 486597a ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51875 --- Ship it! Ship It! - Prasanth_J On Aug. 29, 2014, 6:13 a.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 29, 2014, 6:13 a.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 51c3f2c ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 4cf98d8 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 3f8648b ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 9798cf3 ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java 7cb7c5e ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 486597a ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51754 --- metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java https://reviews.apache.org/r/24472/#comment90343 Why not just use Warehouse.getFileStatusesForSD(tbl.getSd())? It does the same thing. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java https://reviews.apache.org/r/24472/#comment90349 If I undestand correctly, the difference between this method and the one below is FileStatus[]. If so factor out the common code and pass FileStatus[] as parameter. In case of tempTables you can use WareHouse.getFileStatusesFromSD() API to get FileStatus[]. Correct me if I am wrong. ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java https://reviews.apache.org/r/24472/#comment90353 You can reuse the oldCols, newCols List above instead of using iterator. idx in the for loop is unused. ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java https://reviews.apache.org/r/24472/#comment90358 Is there any reason why you are not using FieldSchema's equals() here? ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java https://reviews.apache.org/r/24472/#comment90331 With my comment below (about using fully qualified column names in place of nested map), this function should become substantially small. It will be like.. ss.getTempTableColStats().get(fullyQualifiedColName) ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java https://reviews.apache.org/r/24472/#comment90334 Same can be done here as well. You can get the fully qualfied col name from colStats object which can be used to update state. - Prasanth_J On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51762 --- ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java https://reviews.apache.org/r/24472/#comment90363 This doesn't look good. Can you use MapString, ColumnStatisticsObj instead? with key being fully qualified column name. StatsUtils.getFullyQualifiedColumnName(String dbname, String tablename, String colname) can be used to generate key. - Prasanth_J On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51763 --- ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q https://reviews.apache.org/r/24472/#comment90365 Can you also add a testcase for partitioned table? similar to columnstats_partlvl.q - Prasanth_J On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 8:02 a.m., Prasanth_J wrote: ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q, line 1 https://reviews.apache.org/r/24472/diff/1/?file=655372#file655372line1 Can you also add a testcase for partitioned table? similar to columnstats_partlvl.q Not currently supporting partitioned temp tables. - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51763 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 7:59 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 89 https://reviews.apache.org/r/24472/diff/1/?file=655370#file655370line89 This doesn't look good. Can you use MapString, ColumnStatisticsObj instead? with key being fully qualified column name. StatsUtils.getFullyQualifiedColumnName(String dbname, String tablename, String colname) can be used to generate key. There are a couple of places in the patch where we want to delete all of the column stats for a table, which gets harder to do if you can only look up the stats based on dbname.tabname.colname. How about I get rid of one level of nested maps by using key tabname.dbname - so MapString, MapString, ColumnStatisticsObj? This would give me an easy way to drop all col stats for one table. - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51762 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java, line 202 https://reviews.apache.org/r/24472/diff/1/?file=655367#file655367line202 Why not just use Warehouse.getFileStatusesForSD(tbl.getSd())? It does the same thing. True, this does seem to do the same thing. Will use Warehouse.getFileStatusesForSD(), though with your suggestion below this method will disappear. On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java, line 239 https://reviews.apache.org/r/24472/diff/1/?file=655367#file655367line239 If I undestand correctly, the difference between this method and the one below is FileStatus[]. If so factor out the common code and pass FileStatus[] as parameter. In case of tempTables you can use WareHouse.getFileStatusesFromSD() API to get FileStatus[]. Correct me if I am wrong. Good, suggestion, I think this should work. On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java, line 396 https://reviews.apache.org/r/24472/diff/1/?file=655368#file655368line396 Is there any reason why you are not using FieldSchema's equals() here? FieldSchema.equals() also compares the column comment, which could be changed during alter table. If just the column comment changed the columns are still relatively similar. On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java, line 391 https://reviews.apache.org/r/24472/diff/1/?file=655368#file655368line391 You can reuse the oldCols, newCols List above instead of using iterator. idx in the for loop is unused. Thought the iterators would be better depending on what kind of List was used. I can redo the loop without using idx. - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51754 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51815 --- ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java https://reviews.apache.org/r/24472/#comment90424 This is wrong - deleting all stats on a table when we only want to delete stats for one column. Will fix. - Jason Dere On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 7:59 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java, line 89 https://reviews.apache.org/r/24472/diff/1/?file=655370#file655370line89 This doesn't look good. Can you use MapString, ColumnStatisticsObj instead? with key being fully qualified column name. StatsUtils.getFullyQualifiedColumnName(String dbname, String tablename, String colname) can be used to generate key. Jason Dere wrote: There are a couple of places in the patch where we want to delete all of the column stats for a table, which gets harder to do if you can only look up the stats based on dbname.tabname.colname. How about I get rid of one level of nested maps by using key tabname.dbname - so MapString, MapString, ColumnStatisticsObj? This would give me an easy way to drop all col stats for one table. Getting rid of one level sounds good. - Prasanth_J --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51762 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 8:02 a.m., Prasanth_J wrote: ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q, line 1 https://reviews.apache.org/r/24472/diff/1/?file=655372#file655372line1 Can you also add a testcase for partitioned table? similar to columnstats_partlvl.q Jason Dere wrote: Not currently supporting partitioned temp tables. Will it throw an exception in that case? If so can you add a NegativeCliDriver test just to make sure it throws some exception if used with partitioned tables. - Prasanth_J --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51763 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java, line 396 https://reviews.apache.org/r/24472/diff/1/?file=655368#file655368line396 Is there any reason why you are not using FieldSchema's equals() here? Jason Dere wrote: FieldSchema.equals() also compares the column comment, which could be changed during alter table. If just the column comment changed the columns are still relatively similar. Can you add equalsIgnoreComment() to FieldSchema then? - Prasanth_J --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51754 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 8:02 a.m., Prasanth_J wrote: ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q, line 1 https://reviews.apache.org/r/24472/diff/1/?file=655372#file655372line1 Can you also add a testcase for partitioned table? similar to columnstats_partlvl.q Jason Dere wrote: Not currently supporting partitioned temp tables. Prasanth_J wrote: Will it throw an exception in that case? If so can you add a NegativeCliDriver test just to make sure it throws some exception if used with partitioned tables. Yep, there is already clientnegative/temp_table_partitions.q - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51763 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
On Aug. 28, 2014, 7:56 a.m., Prasanth_J wrote: ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java, line 396 https://reviews.apache.org/r/24472/diff/1/?file=655368#file655368line396 Is there any reason why you are not using FieldSchema's equals() here? Jason Dere wrote: FieldSchema.equals() also compares the column comment, which could be changed during alter table. If just the column comment changed the columns are still relatively similar. Prasanth_J wrote: Can you add equalsIgnoreComment() to FieldSchema then? Unfortunately FieldSchema is a generated class based on the metastore Thrift definition file so we can't really do that. I can add that as a utility method somewhere but that's it. - Jason --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/#review51754 --- On Aug. 26, 2014, 6:37 p.m., Jason Dere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
Re: Review Request 24472: HIVE-7649: Support column stats with temporary tables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24472/ --- (Updated Aug. 26, 2014, 6:37 p.m.) Review request for hive and Prasanth_J. Bugs: HIVE-7649 https://issues.apache.org/jira/browse/HIVE-7649 Repository: hive-git Description --- Update SessionHiveMetastoreClient to get column stats to work for temp tables. Diffs - metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 5a56ced ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java 37b1669 ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java 24f3710 ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java fcfcf42 ql/src/test/queries/clientnegative/temp_table_column_stats.q 9b7aa4a ql/src/test/queries/clientpositive/temp_table_display_colstats_tbllvl.q PRE-CREATION ql/src/test/results/clientnegative/temp_table_column_stats.q.out 4b0c0bc ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out PRE-CREATION Diff: https://reviews.apache.org/r/24472/diff/ Testing --- Thanks, Jason Dere
