HIVE-29203:get_aggr_stats_for doesn't aggregate stats when direct sql… #6089
Conversation
… batch retrieve is enabled
ramitg254
force-pushed
the
HIVE-29203
branch
from
6171f2e to
2a93861
Compare
ramitg254
changed the title
asf-ci-hive
added
tests failed
tests pending
tests passed
and removed
tests pending
tests failed
labels
|
dropped the batched test added earlier as |
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
| enableBitVector, enableKll); | ||
| } | ||
| }); | ||
| return columnStatisticsObjForPartitionsBatch(catName, dbName, tableName, partNames, inputColNames, engine, |
There was a problem hiding this comment.
nit: can we just aggrStatsUseJava directly here and remove the columnStatisticsObjForPartitionsBatch
| areAllPartsFound, useDensityFunctionForNDVEstimation, ndvTuner); | ||
| } | ||
|
|
||
| private List<ColumnStatisticsObj> aggrStatsUseDB(String catName, String dbName, |
There was a problem hiding this comment.
+1 for removing the aggrStatsUseDB, if we enable the batch retrieval, then the stats might not be aggregated per column. If we don't, we might hit the limitation of maximum parameters for PreparedStatement for some dbs.
Let's see how the test going
There was a problem hiding this comment.
do i get it right. we are moving stats aggregation from backend db to Java? what would be the impact on performance?
someone was working on this optimization and now we drop it?
There was a problem hiding this comment.
aggrStatsUseDB can only be used if hive.stats.fetch.bitvector and metastore.stats.fetch.kll are false. Some tests enable them via set command in q files or entire tests suits via hive-site but the default is false. Aggregating the stats at the backend db is usually faster then doing it in java so we can lose performance with this patch in some cases.
- Could you please investigate how to aggregate the results of subsequent
aggrStatsUseDBcalls? - Seems that we have test coverage for this: TestObjectStore.java.testAggrStatsUseDB Should it be removed along with
aggrStatsUseDB?
If we don't, we might hit the limitation of maximum parameters for PreparedStatement for some dbs.
Let's see how the test going
Do we have tests using other dbs than derby and the postgres image?
There was a problem hiding this comment.
@kasakrisz I have added aggregation for aggrStatsUseDB earlier upto the commit b566816 but on later suggestion I removed it
|
|
+1. cc @kasakrisz @deniskuzZ @nrg4878 @saihemanth-cloudera @zhangbutao in case if have any other ideas. |
Hi @ramitg254, Would the tpcds tests fail without this PR? BTW, What is the size of the tpcds test dataset? |
Hi @zhangbutao, without this pr tpcds tests will fail whenever and i think dataset size would be 30tb as I was using |
… batch retrieve is enabled
What changes were proposed in this pull request?
currently flow of query when
hive.metastore.direct.sql.batch.size>0is:aggrColStatsForPartitions -> columnStatisticsObjForPartitions -> columnStatisticsObjForPartitionsBatch -> aggrStatsUseJava -> getPartitionStatsin this case
columnStatisticsObjForPartitionsis also apply batching on partitions list which is not merged further and also this batching for partitions list is not needed asgetPartitionStatsalso applies batching on partitions list which gets merged after batching and get returned viacolumnStatisticsObjForPartitionseventually preventing any redundant entry for a given column NameWhy are the changes needed?
currently in case
hive.metastore.direct.sql.batch.size>0thenList<ColumnStatisticsObj>returned fromcolumnStatisticsObjForPartitionsconsists of multiple entries with similar column name on which merging is not performed resulting in wrong stats.Does this PR introduce any user-facing change?
No
How was this patch tested?
by running tpcds tests locally with setting
hive.metastore.direct.sql.batch.sizeas 1000 inHiveConf.javaandMetastoreConf.java.