kasakrisz commented on code in PR #6782:
URL: https://github.com/apache/hive/pull/6782#discussion_r4142169574
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SizeBasedBigTableSelectorForAutoSMJ.java:
##########
@@ -84,4 +92,45 @@ protected long getSize(HiveConf conf, Partition partition) {
return getSize(conf, size, path);
}
+
+ /**
+ * The size of the partitions a scan reads. A storage handler keeping its
own statistics is asked
+ * for all of them at once: it holds no partition parameters to read one by
one, and the table's
+ * own size stands for every partition rather than for any of them.
+ */
+ protected long getSize(HiveConf conf, Table table, List<Partition>
partitions) {
+ if (!table.isNonNative()) {
+ long total = 0;
+ for (Partition partition : partitions) {
+ total += getSize(conf, partition);
+ }
+ return total;
+ }
+ List<String> partNames =
partitions.stream().map(Partition::getName).toList();
+ Map<String, Map<String, String>> stats =
+ table.getStorageHandler().getAggrBasicStatsFor(table, partNames);
+ long total = 0;
+ for (String partName : partNames) {
+ Map<String, String> partStats = stats.get(partName);
+ String size = partStats != null ?
partStats.get(StatsSetupConst.TOTAL_SIZE) : null;
+ long partSize = NumberUtils.toLong(size, -1);
+ if (partSize < 0) {
+ // a partition it cannot size would leave the total standing for less
than the scan reads,
+ // so the table's own size answers instead: more than the scan reads,
never less
Review Comment:
Could you please rephrase this comment
##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/TestSizeBasedBigTableSelectorForAutoSMJ.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.hadoop.hive.ql.optimizer;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hive.common.StatsSetupConst;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.ql.metadata.HiveStorageHandler;
+import org.apache.hadoop.hive.ql.metadata.Partition;
+import org.apache.hadoop.hive.ql.metadata.Table;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+/**
+ * The big-table choice for an automatic sort-merge join must take a handler
table's size from
+ * the handler's statistics, and must never list the table's location for one.
+ */
+public class TestSizeBasedBigTableSelectorForAutoSMJ {
+
+ private final SizeBasedBigTableSelectorForAutoSMJ selector = new
TableSizeBasedBigTableSelectorForAutoSMJ();
+ private final HiveConf conf = new HiveConf();
+
+ private Table handlerTable(Map<String, String> basicStats) {
+ HiveStorageHandler handler = Mockito.mock(HiveStorageHandler.class);
+ Mockito.when(handler.canProvideBasicStatistics()).thenReturn(true);
+
Mockito.when(handler.getBasicStatistics(Mockito.any())).thenReturn(basicStats);
+ Table table = Mockito.mock(Table.class);
+ Mockito.when(table.isNonNative()).thenReturn(true);
+ Mockito.when(table.getStorageHandler()).thenReturn(handler);
+ return table;
+ }
+
+ @Test
+ public void
handlerTableSizeComesFromItsStatisticsWithoutTouchingTheFilesystem() {
+ Table table = handlerTable(Map.of(StatsSetupConst.TOTAL_SIZE, "12345"));
+
+ Assert.assertEquals(12345, selector.getSize(conf, table));
+ // the location is only asked for on the listing fallback, which a handler
table never takes
+ Mockito.verify(table, Mockito.never()).getPath();
+ }
+
+ @Test
+ public void handlerTableOfUnknownSizeReportsUnknownRatherThanListing() {
+ Table table = handlerTable(Map.of());
+
+ Assert.assertEquals(-1, selector.getSize(conf, table));
+ Mockito.verify(table, Mockito.never()).getPath();
+ }
+
+ @Test
+ public void handlerPartitionsAreSizedTogetherAndSummed() {
+ // the table's own size stands for every partition rather than for any of
them, so the
+ // partitions a scan reads are asked for by name and their sizes added
Review Comment:
Please rephrase this comment.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/TableSizeBasedBigTableSelectorForAutoSMJ.java:
##########
@@ -70,9 +70,7 @@ public int getBigTablePosition(ParseContext parseCtx,
JoinOperator joinOp,
else {
// For partitioned tables, get the size of all the partitions
PrunedPartitionList partsList = PartitionPruner.prune(topOp,
parseCtx, null);
- for (Partition part : partsList.getNotDeniedPartns()) {
- currentSize += getSize(conf, part);
- }
+ currentSize = getSize(conf, table,
List.copyOf(partsList.getNotDeniedPartns()));
Review Comment:
`List.copyOf` is redundant
https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/PrunedPartitionList.java#L86-L88
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/AvgPartitionSizeBasedBigTableSelectorForAutoSMJ.java:
##########
@@ -82,10 +82,7 @@ public int getBigTablePosition(ParseContext parseCtx,
JoinOperator joinOp,
// For partitioned tables, get the size of all the partitions
PrunedPartitionList partsList = PartitionPruner.prune(topOp,
parseCtx, null);
numPartitions = partsList.getNotDeniedPartns().size();
- long totalSize = 0;
- for (Partition part : partsList.getNotDeniedPartns()) {
- totalSize += getSize(conf, part);
- }
+ long totalSize = getSize(conf, table,
List.copyOf(partsList.getNotDeniedPartns()));
Review Comment:
`PrunedPartitionList.getNotDeniedPartns` already returns `List.copyOf`
https://github.com/apache/hive/blob/289d7280cb02afc251de506d65a79a5307b4f229/ql/src/java/org/apache/hadoop/hive/ql/parse/PrunedPartitionList.java#L86-L88
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
