Hive: Expose Snapshot Stats in HMS. by flyrain · Pull Request #4456 · apache/iceberg

flyrain · 2022-03-31T18:24:08Z

cc @aokolnychyi @RussellSpitzer @raptond @karuppayya @szehon-ho @kbendick

anuragmantri

If I understand correctly, is the idea here to get snapshot information just by querying the catalog (in cases where we only have access to catalog)?

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

flyrain · 2022-04-01T17:28:43Z

If I understand correctly, is the idea here to get snapshot information just by querying the catalog (in cases where we only have access to catalog)?

That's correct.

anuragmantri

Thanks for change. It looks harmless to me. I will let others comment.

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java

singhpk234

Thanks @flyrain for this feature :) !!!

singhpk234 · 2022-04-01T18:42:43Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+        parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY,
+            JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()));
+      } catch (JsonProcessingException e) {
+        LOG.warn("Failed to convert snapshot summary to a json string", e);


[minor] Does it makes sense to capture, snapshotId as well for which we got this exception ?

Make sense, will print out snapshot id in the warn message.

singhpk234 · 2022-04-01T18:58:40Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+      parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis()));
+      try {
+        parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY,
+            JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()));


[question] should we take size of summary, also into consideration, As if when we should keep it or is it always optimal to keep it ?

As per my understanding and I may be wrong here, The scenario I have in mind (not sure how practical it is), If some one want's to track changed partition metrics in summary and snapshot had a lot of changed partition and we set CHANGED_PARTITION_COUNT_PROP to a high value, In this scenario the json size could be very large, which is okayish to keep in File, but in DB I am not sure. Your thoughts ?

iceberg/core/src/main/java/org/apache/iceberg/SnapshotSummary.java

Lines 183 to 190 in 86b42fe

setIf(trustPartitionMetrics, builder, CHANGED_PARTITION_COUNT_PROP, changedPartitions.size());

if (trustPartitionMetrics && changedPartitions.size() <= maxChangedPartitionsForSummaries) {

setIf(changedPartitions.size() > 0, builder, PARTITION_SUMMARY_PROP, "true");

for (String key : changedPartitions) {

setIf(key != null, builder, CHANGED_PARTITION_PREFIX + key, partitionSummary(partitionMetrics.get(key)));

}

}

Good question! According to https://issues.apache.org/jira/browse/HIVE-12274 and https://issues.apache.org/jira/browse/HIVE-14145, we can at least store 4000 chars as the values before HMS3.x, and more in HMS3.x depending on the under layer RDMS, at least 32672 bytes. I'm going to cap the summary at 4000 chars.

szehon-ho

Hi @flyrain I think it is useful for hive-based tooling, thanks for this, had some comments and questions.

Would be nice to get a look from @RussellSpitzer @aokolnychyi @pvary too.

szehon-ho · 2022-04-05T17:09:15Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

  private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms";
  private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries";
  private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms";
+  private static final long HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX = 4000;


Should we should expose this configuration, otherwise it may not match what works for all users (different database and backends)? I am not sure what others think.

Ideally, it should be configurable. Not sure it's worth to do that at this moment. I'm open to any suggestion.

Are different hms databases an issue? I would hope this would be more or less standardized

I guess my comment is not complete, @flyrain's analysis in #4456 (comment) is the exact one. Older versions of Hive had the 4000 limit (due to need to support some older databases), and newer versions increased to 32672.

So, if we don't have some kind of flag, we lose the full potential of current HMS versions just to support the older ones.

It would be nice to rely on a configuration in hive, like MAX_STRING_SIZE, and we can read it from somewhere like hive-site.xml, I didn't find any though. It's a bit weird that we got an Iceberg configuration item indicating the max value size of a HMS, maybe that's fine, the best place to hold it is the catalog config, something like spark.sql.catalog.spark_catalog.max_table_property_size. Does it make sense?

Hm, yea I'm also not entirely sure what the right thing to do here is. Yea it's hidden a bit deeply in the package.jdo file in Hive..

I see we do have some Hive specific configs in this file right (like iceberg.hive.lock-timeout-ms for example), and also ConfigProperties.java? Could it be with that prefix (ie, iceberg.hive.summary.stats.max.size), with default value and upper bound 32672? Or if we don't want to give too much complexity to user, maybe iceberg.hive.legacy.table.parameter.size which is just a boolean controlling whether its 32672 or 4000.

Sounds good. The only downside is we cannot separate them when two hive catalogs are configured, but that's probably fine. iceberg.hive.lock-timeout-ms couldn't either.

szehon-ho · 2022-04-05T17:10:59Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+        if (summary.length() <= HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX) {
+          parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary);
+        } else {
+          LOG.warn("Not expose the current snapshot({}) summary in HMS since it exceeds {} characters",


nit: "Not exposing".

szehon-ho · 2022-04-05T17:18:14Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

  }

+  private void setSnapshotStats(TableMetadata metadata, Map<String, String> parameters) {
+    Snapshot currentSnapshot = metadata.currentSnapshot();


I think we need a way to clear the properties if they are not there anymore in current snapshot (like original code setHmsProperties). For example if the summary becomes suddenly over the length in new snapshot, then it will not override the old summary and would be wrong.

Fixed in the new commit.

szehon-ho · 2022-04-05T17:22:52Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+
+      // check whether parameters are in expected state
+      Map<String, String> parameters = hmsTable.getParameters();
+      Assert.assertEquals("0", parameters.get(TableProperties.SNAPSHOT_COUNT));


Can we put some error messages in these asserts, like in other tests?

I'm not sure it is necessary. The code here can explain itself quite well.

szehon-ho · 2022-04-05T17:45:39Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+      try {
+        String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary());
+        if (summary.length() <= HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX) {
+          parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary);


Also had one concern, there's some repeating information (setHmsTableParameters puts some but not all of the summary). Did we ever consider just inlining these fields from the summary like they did over there?

If it's a lot of properties, the json makes sense, just thinking its a shame there is a bit of repeat.

These duplicated properties(numFiles, numRows, totalSize) are handy for existing HMS consumers, since they are hive table properties, the existing tool can use them out-of-box. It's nice to keep them as exception cases. However, the snapshot summary have much more stats. It may not be a good idea to split them into individual HMS table properties. For example, any summary change requires a code change here change in that case.

RussellSpitzer · 2022-04-06T16:01:38Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

  }
+
+  @Test
+  public void testSnapshotStatsTableProperties() throws Exception {


We should probably have a test with the string exceeding the property size threshold, checking to make sure the behavior is as we expect.

Added a test case for that

RussellSpitzer

I had a few minor comments, but @szehon-ho is the Hive expert so when he is +1 I think this is good to merge

flyrain · 2022-04-06T17:35:49Z

I had a few minor comments, but @szehon-ho is the Hive expert so when he is +1 I think this is good to merge

Thanks for the review. Let me address some comments before the merge.

flyrain · 2022-04-06T19:43:43Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

  private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms";
  private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries";
  private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms";
+  private static final String HIVE_LEGACY_TABLE_PARAMETER_SIZE = "iceberg.hive.legacy.table.parameter.size";


The current name doesn't look like a boolean type. I'm open to suggestion. iceberg.hive.is.legacy.table.parameter.size?

hive.use.legacy.table.parameter.size?

flyrain · 2022-04-07T17:24:29Z

Talked to @szehon-ho offline. We will give a default value for the HIVE_TABLE_PROPERTY_SIZE, while allowing user to configure it with different number due to a variety of HMS backends.

Oracle DB's varchar2 supports 32767 bytes now, by setting the configuration parameter MAX_STRING_SIZE to EXTENDED. (source)
Postgres supports a max of 1GB for character datatype. (source)
MySQL can support upto 65535 bytes for the entire row. So long as the PARAM_KEY value + PARAM_VALUE is less than 65535, we should be good. (source)
SQL Server's varchar max length is 8000 and can go beyond using "varchar(max)" with the same limitation as MySQL being 65535 bytes for the entire row. (source)
Derby's varchar can be upto 32672 bytes. (source)

https://issues.apache.org/jira/browse/HIVE-12274

szehon-ho

@flyrain thanks for changes, looks mostly good, just a few more nits

szehon-ho · 2022-04-07T18:07:44Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

        conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT);
    long tableLevelLockCacheEvictionTimeout =
        conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT);
+    this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_SIZE_MAX, HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT);


Nit: can you move it up one line so not to break flow of the tableLevelLockCacheEvictionTimeout statement?

szehon-ho · 2022-04-07T18:10:57Z

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java

+      summary.put(String.valueOf(i), "value");
+    }
+    long summarySize = JsonUtil.mapper().writeValueAsString(summary).length();
+    Assert.assertTrue(summarySize > 4000 && summarySize < 32672);


Nit: i think no need for the upper limit check, right? We are not really saving in Hive.

This is to make sure the new limit takes affect.

conf.set("iceberg.hive.table.parameter.size.max", "4000");

Add a comment in the code

Hm, I meant, can't we just do

Assert.assertTrue("Summary size should be greater than limit", summarySize > 4000).

To me the test is checking whether you can persist something beyond 4000 chars right? The fact that it's below or above 32627 chars should not trigger now that we changed limit to 4000 right? Then, no need for the additional comment, if this is the case. Let me know if I am mistaken though.

The main purpose is to test whether we save the summary in HMS parameters when the size exceeds the limit. Besides, I also want to test if the limit has changed from 32627 to 4000. That's why I check both.

Assert.assertTrue(summarySize > 4000 && summarySize < 32672);

szehon-ho · 2022-04-07T18:11:24Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

  private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries";
  private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms";
+  private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.table.parameter.size.max";
+  private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672;


Should we add a comment, giving some reference to when it was changed in Hive?

Thanks, nit: how about "iceberg.hive.max.table.parameter.size".

Also, regarding the comment, it's kind of based on backend but Hive had a global limit of 4000 regardless of database to allow it to support across different database, so maybe "For Hive versions below 2.3, max table parameter size is 4000 characters, see https://issues.apache.org/jira/browse/HIVE-12274"?

Made the change

one more nit: can the variable match the changed string

Actually sorry about this , I just noticed other flags, weird how they do dash after "iceberg.hive". How about HIVE_TABLE_PROPERTY_MAX_SIZE / 'iceberg.hive.table-property-max-size" (And fixes the correct term to be table-property).

Made the change.

flyrain · 2022-04-07T22:46:41Z

The failed test should not be related.

org.apache.iceberg.flink.TestFlinkUpsert > testPrimaryKeyFieldsAtEndOfTableSchema[catalogName=testhadoop, baseNamespace=default, format=PARQUET, isStreaming=true] FAILED
    java.lang.AssertionError: 
    Expecting:
      [+I[3, bbb, 2022-03-01], +I[1, aaa, 2022-03-01]]
    to contain exactly in any order:
      [+I[2, aaa, 2022-03-01], +I[3, bbb, 2022-03-01]]
    elements not found:
      [+I[2, aaa, 2022-03-01]]
    and elements not expected:
      [+I[1, aaa, 2022-03-01]]
        at org.apache.iceberg.flink.TestHelpers.assertRows(TestHelpers.java:137)
        at org.apache.iceberg.flink.TestFlinkUpsert.testPrimaryKeyFieldsAtEndOfTableSchema(TestFlinkUpsert.java:262)

szehon-ho

Looks good to me. Just left one comment if you have time, otherwise it's fine. Probably can restart the test too.

szehon-ho · 2022-04-07T23:40:38Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+      if (summary.length() <= maxHiveTablePropertySize) {
+        parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary);
+      } else {
+        parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY);


Optional: we can actually put the parameters.remove() for all the properties above the parameters.put(), here we are doing the remove three times.

Good idea. Made the change.

szehon-ho · 2022-04-08T16:50:11Z

Merged, thanks @flyrain for the changes

flyrain · 2022-04-08T17:36:20Z

Thanks @szehon-ho! Thanks all for the review, @singhpk234, @RussellSpitzer, @anuragmantri.

(cherry picked from commit 61f0854)

Core: Add Reserved Snapshot Stats Table Property and Expose in HMS.

98a9b73

github-actions bot added core hive labels Mar 31, 2022

Fix the unit test failure

5fe283c

github-actions bot added the MR label Mar 31, 2022

anuragmantri reviewed Apr 1, 2022

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java Show resolved Hide resolved

anuragmantri approved these changes Apr 1, 2022

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java Show resolved Hide resolved

singhpk234 reviewed Apr 1, 2022

View reviewed changes

flyrain added 2 commits April 3, 2022 18:02

Doesn't expose the summary if its length is more than 4000 chars.

c7b0e62

Doesn't expose the summary if its length is more than 4000 chars.

1d96a1b

szehon-ho reviewed Apr 5, 2022

View reviewed changes

RussellSpitzer reviewed Apr 6, 2022

View reviewed changes

RussellSpitzer approved these changes Apr 6, 2022

View reviewed changes

flyrain added 2 commits April 6, 2022 12:34

Resolve comments

8eac228

Resolve comments

3a11d9b

flyrain commented Apr 6, 2022

View reviewed changes

flyrain added 2 commits April 6, 2022 13:00

Simplify the test case testSetSnapshotSummary

72bd3be

Allow user to config the max size

4daa05a

szehon-ho reviewed Apr 7, 2022

View reviewed changes

flyrain added 4 commits April 7, 2022 11:39

Resolve comments

8f5edaa

Resolve comments

cd726fe

Resolve comments

1e0d23f

Resolve comments

a3bea2f

szehon-ho approved these changes Apr 7, 2022

View reviewed changes

szehon-ho closed this Apr 7, 2022

szehon-ho reopened this Apr 7, 2022

Resolve comments

617d665

szehon-ho approved these changes Apr 8, 2022

View reviewed changes

szehon-ho merged commit 61f0854 into apache:master Apr 8, 2022

flyrain mentioned this pull request Apr 15, 2022

Hive: Expose default partition spec and sort order in HMS. #4546

Merged

InvisibleProgrammer mentioned this pull request Dec 14, 2022

HIVE-26822: port changes before spotless apache/hive#3857

Closed

InvisibleProgrammer mentioned this pull request Jan 3, 2023

HIVE-26808: port iceberg catalog changes apache/hive#3907

Merged

flyrain mentioned this pull request Apr 25, 2023

Flink will throw MetaException when hive table property value is larger than 4000 #7113

Closed

sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 9, 2023

Hive: Expose Snapshot Stats in HMS (apache#4456)

4bfcd82

(cherry picked from commit 61f0854)

	setIf(trustPartitionMetrics, builder, CHANGED_PARTITION_COUNT_PROP, changedPartitions.size());

	if (trustPartitionMetrics && changedPartitions.size() <= maxChangedPartitionsForSummaries) {
	setIf(changedPartitions.size() > 0, builder, PARTITION_SUMMARY_PROP, "true");
	for (String key : changedPartitions) {
	setIf(key != null, builder, CHANGED_PARTITION_PREFIX + key, partitionSummary(partitionMetrics.get(key)));
	}
	}

Conversation

flyrain commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anuragmantri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flyrain commented Apr 1, 2022

Uh oh!

anuragmantri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Apr 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain commented Mar 31, 2022 •

edited

Loading

singhpk234 Apr 1, 2022 •

edited

Loading

flyrain Apr 1, 2022 •

edited

Loading

szehon-ho left a comment •

edited

Loading

szehon-ho Apr 6, 2022 •

edited

Loading

flyrain Apr 5, 2022 •

edited

Loading

flyrain Apr 6, 2022 •

edited

Loading

flyrain commented Apr 7, 2022 •

edited

Loading

szehon-ho Apr 7, 2022 •

edited

Loading

szehon-ho Apr 7, 2022 •

edited

Loading

szehon-ho Apr 7, 2022 •

edited

Loading

szehon-ho Apr 7, 2022 •

edited

Loading