You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/02/01 17:07:17 UTC
[GitHub] [hive] kasakrisz opened a new pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
kasakrisz opened a new pull request #2991:
URL: https://github.com/apache/hive/pull/2991
### What changes were proposed in this pull request?
When inserting into partitioned tables the list of affected partitions are provided by Dynamic partitioning when updating Statistics. Reload the Partition objects from HMS when `BasicStatsTask` gets them.
### Why are the changes needed?
All write branches/entities of sql statements has its own `BasicStatsTask` instance which updates the stats of the partition when the statement finished execution. The Tasks are executed sequentially however the state of partitions were loaded only once from metastore by Dynamic partitioning so each `BasicStatsTask` updated the same staring state.
### Does this PR introduce _any_ user-facing change?
Yes. Incorrect results are fixed.
### How was this patch tested?
```
mvn test -Dtest.output.overwrite -DskipSparkTests -Dtest=TestMiniLlapLocalCliDriver -Dqfile=stats_part_multi_insert.q -pl itests/qtest -Pitests
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r802348206
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,9 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ List<String> partNames = list.stream().map(Partition::getName).collect(Collectors.toList());
Review comment:
Initially I didn't want to refactor this method just provide a simple fix to the correctness issue.
Thanks for your suggestions these are small changes and makes sense to me so I'm open to expand a bit the scope of this patch.
My last commit is also a refactor: since the stream is created over the `dbPartSpecs` the `list` object became unnecessary in this if branch. So I decided to remove it completely.
Let's see how ptest performs with this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r800659669
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
This change is kind of reverting https://issues.apache.org/jira/browse/HIVE-15250. Obviously correctness is more important than performance but I am wondering if we explored other ways to fix the problem.
@rbalamohan since you worked on HIVE-15250 you may want to have a look into the changes here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r801604148
##########
File path: ql/src/test/queries/clientpositive/stats_part_multi_insert.q
##########
@@ -0,0 +1,18 @@
+-- Test multi inserting to the same partition
+
+create table source(p int, key int,value string);
+insert into source(p, key, value) values (101,42,'string42');
+
+create table stats_part(key int,value string) partitioned by (p int);
+
+from source
+insert into stats_part select key, value, p
+insert into stats_part select key, value, p;
+
+select p, key, value from stats_part;
+desc formatted stats_part;
+select count(*) from stats_part;
Review comment:
To keep the test functional even if the default value of `hive.compute.query.using.stats` changes in the future I would add an explicit set to true just before the select query.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
Given that we are dealing with a correctness problem here, I would be inclined to commit this fix as is right now and treat possible improvements if any in a follow up keeping in mind that the performance overhead of `getPartitionByNames` is not measured.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
Maybe it would be slightly more readable like below:
```suggestion
// Reload partition metadata because another BasicStatsTask instance may have updated the stats.
List<String> partNames = dpPartSpecs.stream().map(Partition::getName).collect(Collectors.toList());
list.addAll(db.getPartitionsByNames(table, partNames));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] rbalamohan commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r801136555
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
getPartitionByNames is a single call unlike 2000+ getPartition() calls (Which internally loads table object everytime etc). It will still add up to runtime depending on the number of partitions for regular insert queries.
One option could be to figure out if it is multi-insert at the time of sem analyzer and pass it on to DynamicPartitionCtx; getPartitionByNames() could be invoked only in those conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r800666998
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
Truth is that things are a bit different here (in comparison with the state before HIVE-15250) in the sense that we are calling `getPartitionsByNames` instead of `getPartition` so if I understand well it will be only one call to DB and not multiple so maybe the perf overhead is not as important as it was in HIVE-15250.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz merged pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
kasakrisz merged pull request #2991:
URL: https://github.com/apache/hive/pull/2991
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r801726004
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,9 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ List<String> partNames = list.stream().map(Partition::getName).collect(Collectors.toList());
Review comment:
I see you applied part of my suggestion, thanks! Any reason why you prefer to create a stream over `list` instead of `dbPartSpecs`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,9 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
Review comment:
The comment here is a bit misleading since we do not load anything here. I think it suffices to just keep the new comment about reloading partitions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #2991: HIVE-25918: Invalid stats after multi inserting into the same partition
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #2991:
URL: https://github.com/apache/hive/pull/2991#discussion_r800706374
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/stats/BasicStatsTask.java
##########
@@ -512,6 +513,8 @@ private String toString(Map<String, String> parameters) {
if (dpPartSpecs != null) {
// load the list of DP partitions and return the list of partition specs
list.addAll(dpPartSpecs);
+ // Reload partition metadata because another BasicStatsTask instance may have updated the stats.
+ list = db.getPartitionsByNames(table, list.stream().map(Partition::getName).collect(Collectors.toList()));
Review comment:
I think these are thrift API calls from HS2 to HMS and the number of calls depend on the number of partitions and `hive.metastore.batch.retrieve.max`.
Also there is some communication from HMS to backend db which is also done in batches.
I explored another way:
* In the StatsTask instead of applying the stats on the Partition objects collect it to a delta objects.
* Implement a HMS call and backend DB call to update stats based on the collected delta objects.
However this would be a bigger change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org