> On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, line > > 5920 > > <https://reviews.apache.org/r/24176/diff/2/?file=648979#file648979line5920> > > > > Should the client (StatsAnnotation) have a fallback mechanism? If > > directSQL is not available or turned off?
I have reservations about failing back to slow path in such cases. This api is used purely for planning purposes(stats annotation). Currently, we already deal with cases when no stats are present at all. We also know fetching stats via old api can take few seconds once # of partitions crosses few thousands. IMO, if stats cant be fetched quickly enough than we should deal that scenario as if stats are not available at all. Spending precious seconds in retrieving stats is not useful, I think. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java, > > line 1826 > > <https://reviews.apache.org/r/24176/diff/2/?file=648976#file648976line1826> > > > > I assume this doesn't aggregate the basic stats like #rows, #files, > > file_size, raw_data_size from PARTITION_PARAMS. If so can you rename this > > method to getAggrColStatsForPartitions(). Statistics Annotation may also > > need aggregated basic stats. OK, will rename the method. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java, > > line 964 > > <https://reviews.apache.org/r/24176/diff/2/?file=648978#file648978line964> > > > > Same here. API name to be explicitly say we are fetching column stats. > > getPartitionColStats()? Ok, will rename the method. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java, line 2580 > > <https://reviews.apache.org/r/24176/diff/2/?file=648983#file648983line2580> > > > > Same comment about the API naming. Will do. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java, line 219 > > <https://reviews.apache.org/r/24176/diff/2/?file=648985#file648985line219> > > > > I think we should leave the state as PARTIAL in both cases. > > 1) When some partitions doesn't have column stats > > 2) When some columns doesn't have stats. > > > > And probably have debug log for both so that we can atleast know what > > we are missing. Though StatsAnnotation doesn't worry about 1) its good to > > know that we are missing something from explain output. Thats what I have done, I think. Reason for change in .q.out is because currently api doesnt return # of partitions correctly. See, my TODO comments in HiveMetaStore::get_aggr_stats_for(). Once that is fixed, .q.out will correctly reflect column stats state. But since, StatsAnnotation doesnt make use of this info, I am proposing to fix this in a follow-up. I will add debug info in this one. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java, line 227 > > <https://reviews.apache.org/r/24176/diff/2/?file=648985#file648985line227> > > > > Will there be any performance gain having JDO fallback vs client side > > fallback? See, my previous comments. Either JDO fallback or client side fallback wont be fast enough to justify falling to them. > On Aug. 2, 2014, 1:18 a.m., Prasanth_J wrote: > > ql/src/test/results/clientpositive/annotate_stats_part.q.out, line 1181 > > <https://reviews.apache.org/r/24176/diff/2/?file=648986#file648986line1181> > > > > The change from my above comment should revert this back. See, my previous comments. Underlying problem is count of partitions returned from Metastore. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24176/#review49414 ----------------------------------------------------------- On Aug. 2, 2014, 12:23 a.m., Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24176/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2014, 12:23 a.m.) > > > Review request for hive and Prasanth_J. > > > Bugs: HIVE-7587 > https://issues.apache.org/jira/browse/HIVE-7587 > > > Repository: hive-git > > > Description > ------- > > Fetch aggregated stats from MetaStore > > > Diffs > ----- > > metastore/if/hive_metastore.thrift 55f41db > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 3b778ee > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 53165d0 > metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > 957b976 > metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h f352cd5 > metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp a6a40fd > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/AddPartitionsRequest.java > 4547970 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/AddPartitionsResult.java > 68a4219 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/AggrStats.java > PRE-CREATION > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ColumnStatistics.java > 6aecf26 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/DropPartitionsResult.java > a4ae892 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/EnvironmentContext.java > ed464d6 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Function.java > 781281a > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetOpenTxnsInfoResponse.java > b782d32 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetOpenTxnsResponse.java > d549ce9 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPrincipalsInRoleResponse.java > 3ef6224 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetRoleGrantsForPrincipalResponse.java > 3ddc1ac > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/HeartbeatTxnRangeResponse.java > f3e3c07 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/HiveObjectRef.java > b22b211 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/LockRequest.java > cdf6f30 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/OpenTxnsResponse.java > 54955c6 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Partition.java > 7d29d09 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PartitionsByExprResult.java > 5ea5a1b > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PartitionsStatsRequest.java > 80a151a > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PartitionsStatsResult.java > 537db47 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PrincipalPrivilegeSet.java > 0c9518a > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PrivilegeBag.java > 4285ed8 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/RequestPartsSpec.java > 2fcb216 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Schema.java > 58e9028 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ShowCompactResponse.java > b962e27 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ShowLocksResponse.java > 1399f8b > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/SkewedInfo.java > ab5c0ed > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/StorageDescriptor.java > 813b4f0 > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java > 484bd6a > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/TableStatsRequest.java > ddfcccc > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/TableStatsResult.java > e37b75c > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > 1e0cdea > > metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Type.java > 1882b57 > metastore/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php b74e54e > metastore/src/gen/thrift/gen-php/metastore/Types.php 4d4ab84 > metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > 6fef2cf > metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 2a2e443 > metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py c71b7b7 > metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb e21f662 > metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 58b9c0e > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > b74868b > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 4c9a597 > metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > d6e849f > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > a23d122 > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 4f186f4 > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 2379ce7 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 396eb4e > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > 80c3b2b > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java a7e50ad > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/stats/annotation/StatsRulesProcFactory.java > 9620e62 > ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java 818590a > ql/src/test/results/clientpositive/annotate_stats_part.q.out 043af14 > > Diff: https://reviews.apache.org/r/24176/diff/ > > > Testing > ------- > > Existing annotate_stats* tests. > > > Thanks, > > Ashutosh Chauhan > >
