ayushtkn commented on code in PR #4348:
URL: https://github.com/apache/hive/pull/4348#discussion_r1211099795
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java:
##########
@@ -173,6 +173,10 @@ public String getAggregator(Configuration conf) {
public static final String FALSE = "false";
+ public static final String HIVE_ICEBERG_STATS_SOURCE =
"hive.iceberg.stats.source";
+
+ public static final String ICEBERG_STATS_SOURCE = "iceberg";
Review Comment:
use HiveMetaHook.ICEBERG;
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java:
##########
@@ -294,6 +297,81 @@ public void testUpdateTableStatsSlow_doesNotUpdateStats()
throws TException {
verify(wh, never()).getFileStatusesForUnpartitionedTable(db, tbl2);
}
+ /**
+ * Verify that updateTableStatsForCreateTable() does not invoke calculation
of table statistics when
+ * <ol>
+ * <li>the table is an Iceberg table</li>
+ * </ol>
+ */
+ @Test
+ public void testUpdateTableStatsForCreateTable_doesNotInvokeStatsCalc()
throws TException {
+ // Iceberg table & Iceberg stats source is Iceberg => doesn't invokes
stats calculation
+ Map<String, String> params = new HashMap<>(paramsWithStats);
+ Warehouse wh = mock(Warehouse.class);
+
+ Table tbl1 = new TableBuilder()
+ .setDbName(DB_NAME)
+ .setTableName(TABLE_NAME)
+ .addCol("id", "int")
+ .setTableParams(params)
+ .build(null);
+ // Add Stats for create table
+ HashMap<String, List<ByteBuffer>> dict = new HashMap<>();
+ ByteBuffer buffer =
ByteBuffer.wrap("{\"enabled\":true,\"isIcebergTable\":true,\"columnNames\":[\"id\"]}".getBytes());
+ dict.put(StatsSetupConst.STATS_FOR_CREATE_TABLE, Arrays.asList(buffer));
Review Comment:
Does ``Collections.singletonList(buffer)`` work?
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java:
##########
@@ -294,6 +297,81 @@ public void testUpdateTableStatsSlow_doesNotUpdateStats()
throws TException {
verify(wh, never()).getFileStatusesForUnpartitionedTable(db, tbl2);
}
+ /**
+ * Verify that updateTableStatsForCreateTable() does not invoke calculation
of table statistics when
+ * <ol>
+ * <li>the table is an Iceberg table</li>
+ * </ol>
+ */
+ @Test
+ public void testUpdateTableStatsForCreateTable_doesNotInvokeStatsCalc()
throws TException {
+ // Iceberg table & Iceberg stats source is Iceberg => doesn't invokes
stats calculation
+ Map<String, String> params = new HashMap<>(paramsWithStats);
+ Warehouse wh = mock(Warehouse.class);
+
+ Table tbl1 = new TableBuilder()
+ .setDbName(DB_NAME)
+ .setTableName(TABLE_NAME)
+ .addCol("id", "int")
+ .setTableParams(params)
+ .build(null);
+ // Add Stats for create table
+ HashMap<String, List<ByteBuffer>> dict = new HashMap<>();
+ ByteBuffer buffer =
ByteBuffer.wrap("{\"enabled\":true,\"isIcebergTable\":true,\"columnNames\":[\"id\"]}".getBytes());
+ dict.put(StatsSetupConst.STATS_FOR_CREATE_TABLE, Arrays.asList(buffer));
+ tbl1.setDictionary(new ObjectDictionary(dict));
+
+ assertFalse(MetaStoreServerUtils.updateTableStatsForCreateTable(wh, db,
tbl1, null,
+ MetastoreConf.newMetastoreConf(), new Path("/tmp/0"), false));
+ }
+
+ /**
+ * Verify that updateTableStatsForCreateTable() invokes calculation of table
statistics when
+ * <ol>
+ * <li>the table is not an Iceberg table</li>
+ * </ol>
+ */
+ @Test
+ public void testUpdateTableStatsForCreateTable_invokeStatsCalc() throws
TException {
Review Comment:
nit
``Underscore`` in test name isn't a common practice can you use CamelCasing
for the name
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java:
##########
@@ -511,12 +513,12 @@ public static void clearQuickStats(Map<String, String>
params) {
params.remove(StatsSetupConst.NUM_ERASURE_CODED_FILES);
}
- public static void updateTableStatsForCreateTable(Warehouse wh, Database db,
Table tbl,
+ public static boolean updateTableStatsForCreateTable(Warehouse wh, Database
db, Table tbl,
Review Comment:
If it is only for unit test, you need to find out a another way, you are
changing the return type of a public method, I don't think it is a compatible
change.
It is functionally compatible, say anyone calling
updateTableStatsForCreateTable() expecting a void will now get a boolean but he
can ignore.
But this change isn't `Binary Compatible`, if the call is across Jars, at
binary level it would be like removed a method with void and added another
method with boolean as return type. So, those callers will fail with
NoSuchMethod error, and for a UT it isn' required
Put some log or something like that and grep that in the tests...
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java:
##########
@@ -173,6 +173,10 @@ public String getAggregator(Configuration conf) {
public static final String FALSE = "false";
+ public static final String HIVE_ICEBERG_STATS_SOURCE =
"hive.iceberg.stats.source";
Review Comment:
the conf is defined in HiveConf as well, either figure out to use that
everywhere if not possible, change that HiveConf definition to use this
```
HIVE_ICEBERG_STATS_SOURCE(StatsSetupConst.HIVE_ICEBERG_STATS_SOURCE,
"iceberg",
"Use stats from iceberg table snapshot for query planning. This has
two values metastore and iceberg"),
```
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java:
##########
@@ -294,6 +297,81 @@ public void testUpdateTableStatsSlow_doesNotUpdateStats()
throws TException {
verify(wh, never()).getFileStatusesForUnpartitionedTable(db, tbl2);
}
+ /**
+ * Verify that updateTableStatsForCreateTable() does not invoke calculation
of table statistics when
+ * <ol>
+ * <li>the table is an Iceberg table</li>
+ * </ol>
+ */
+ @Test
+ public void testUpdateTableStatsForCreateTable_doesNotInvokeStatsCalc()
throws TException {
+ // Iceberg table & Iceberg stats source is Iceberg => doesn't invokes
stats calculation
+ Map<String, String> params = new HashMap<>(paramsWithStats);
+ Warehouse wh = mock(Warehouse.class);
+
+ Table tbl1 = new TableBuilder()
+ .setDbName(DB_NAME)
+ .setTableName(TABLE_NAME)
+ .addCol("id", "int")
+ .setTableParams(params)
+ .build(null);
+ // Add Stats for create table
+ HashMap<String, List<ByteBuffer>> dict = new HashMap<>();
+ ByteBuffer buffer =
ByteBuffer.wrap("{\"enabled\":true,\"isIcebergTable\":true,\"columnNames\":[\"id\"]}".getBytes());
+ dict.put(StatsSetupConst.STATS_FOR_CREATE_TABLE, Arrays.asList(buffer));
+ tbl1.setDictionary(new ObjectDictionary(dict));
+
+ assertFalse(MetaStoreServerUtils.updateTableStatsForCreateTable(wh, db,
tbl1, null,
+ MetastoreConf.newMetastoreConf(), new Path("/tmp/0"), false));
+ }
+
+ /**
+ * Verify that updateTableStatsForCreateTable() invokes calculation of table
statistics when
+ * <ol>
+ * <li>the table is not an Iceberg table</li>
+ * </ol>
+ */
+ @Test
+ public void testUpdateTableStatsForCreateTable_invokeStatsCalc() throws
TException {
+ // Non-Iceberg table => invoke stats calculation
+ Map<String, String> params = new HashMap<>(paramsWithStats);
+ Warehouse wh = mock(Warehouse.class);
+
+ Table tbl1 = new TableBuilder()
+ .setDbName(DB_NAME)
+ .setTableName(TABLE_NAME)
+ .addCol("id", "int")
+ .setTableParams(params)
+ .build(null);
+ // Add Stats for create table
+ HashMap<String, List<ByteBuffer>> dict1 = new HashMap<>();
+ ByteBuffer buffer1 =
ByteBuffer.wrap("{\"enabled\":true,\"isIcebergTable\":false,\"columnNames\":[\"id\"]}".getBytes());
+ dict1.put(StatsSetupConst.STATS_FOR_CREATE_TABLE, Arrays.asList(buffer1));
Review Comment:
``Collections.singletonList(buffer1)`` and in other places as well
--
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]
