Hive: Arrange common part of the code for Iceberg View. by nk1506 · Pull Request #10001 · apache/iceberg

nk1506 · 2024-03-19T09:05:24Z

Hive considers iceberg-view just another type of Hive-Table. So Iceberg-View on Hive should have same strategy for commit as Iceberg-Table on Hive. As part of this refactoring all the common code for table which can be used by view too has been moved to common places. Now HiveOperationsBase has all the common code for committing the iceberg-table and iceberg-view.

This change also define a new abstract class BaseMetastoreOperations which can contains common code from BaseMetastoreTableOperations and BaseViewOperations .

Motivation behind this change to have less diff for implementing Iceberg-view on Hive.

nk1506 · 2024-03-19T09:19:35Z

  protected void doCommit(TableMetadata base, TableMetadata metadata) {
    boolean newTable = base == null;
    String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata);
-    boolean hiveEngineEnabled = hiveEngineEnabled(metadata, conf);


All this common code has been moved to HiveOperationsBase .

see my comment in https://github.com/apache/iceberg/pull/10001/files#r1533430710. Please revert these changes here. Once view support is in, we can revisit what we'll extract out

nastra · 2024-03-19T11:03:56Z

          && e.getMessage().contains(String.format("new table %s already exists", to))) {
-        throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-            "Table already exists: %s", to);
+        throwErrorForExistedToContent(fromIdentifier, removeCatalogName(toIdentifier));


I don't think we want to do this here, as this would do another call to retrieve the table. I would leave the code here as it was before

Motivation behind this changes is
ViewCatalogTests expects error message like Cannot rename %s to %s. %s already exists. It should explicitly tell the upstream TO tableType.

Also we are making only one call to retrieve TO content.
Nevertheless, What do you propose to match the error messages for ViewCatalogTests ?

what I mean is that we shouldn't be doing whather it's doing in throwErrorForExistedToContent() at this point and just throw the AlreadyExistsException as it was done previously. We don't want do add new functionality in this PR IMO

nastra · 2024-03-19T11:05:51Z

            Maps.newHashMap(),
-            null,
-            null,
+            properties.getOrDefault(HiveCatalog.VIEW_ORIGINAL_TEXT, null),


why is this adding view-specific things?

I think this might not be specific to Hive-View. This is another field in Hive-Table . IMO, This field either we can ignore for both iceberg-table and view or make it optional with properties.
Ref: https://github.com/apache/hive/blame/df45194268ffb10f9f7aae0ab4e3aec35a7b31d8/standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java#L23

I think this PR should be a pure refactoring and not add new things/functionality. The current code on main doesn't look at this property, so why do we need to check this property as part of this PR?

nk1506 · 2024-03-21T05:35:16Z

@nastra , Please let me know if there are more comments ?

nastra · 2024-03-21T07:50:04Z

+   * @param newMetadataLocation newly written metadata location
+   * @return true if the new metadata location presents with current or previous metadata files.
+   */
+  protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {


why does this have to be protected?

Not Private , bcoz It should be available outside of the core-package. Not public as it should be only scoped too child class. Do we need public privilege here ?

I think this should be private. Why would it need to be available for subclasses? I don't think subclasses would call this code?

It depends how the checkCommitStatus is being called. It it is using
BaseMetastoreTableOperations.checkCommitStatus it can be private . it it is using BaseMetastoreOperations.checkCommitStatus it should be protected.

nastra · 2024-03-21T07:52:28Z

+   * Checks the new metadata location presents or not after refreshing the table.
+   *
+   * @param newMetadataLocation newly written metadata location
+   * @return true if the new metadata location presents with current or previous metadata files.


Suggested change

* @return true if the new metadata location presents with current or previous metadata files.

* @return true if the new metadata location is the current metadata location or present within previous metadata files.

please also update the javadoc in L317 with a similar sentence as here

nastra · 2024-03-21T08:21:03Z

+      TableIdentifier from,
+      TableIdentifier originalTo,
+      HiveOperationsBase.ContentType contentType) {
+    Preconditions.checkArgument(isValidIdentifier(from), "Invalid identifier: %s", from);


this is changing existing behavior. Previously it would throw NoSuchTableException

Intent here was to have common exception for Table/View. But I think with View impl we will have to revisit this.

nastra · 2024-03-21T08:35:00Z

  }
+
+  @SuppressWarnings("checkstyle:CyclomaticComplexity")
+  default void commitWithHiveLock(


can we revert any changes in this class starting from L276 to L473? I feel like we're extracting too much functionality into a common place and some things are slightly different for tables/views later.
That's why I think it would be better to revert L276 to L473 at this point. In the first iteration of view support it's probably ok if some things are duplicated and once View support for Hive is in, we can do another round of refactoring to extract what's really common

nk1506 · 2024-03-21T11:48:12Z

-      return null;
-    }
-  }
+  public void setHmsParameters(


It has been split into two parts , one is specific to Table like snapshot,stats, etc. Other pat is common parameters as schema, location, uuid.

please revert this to how it was before

…able/View with other comments.

nk1506 · 2024-03-21T12:32:16Z

            tableName,
            e);
-        commitStatus = checkCommitStatus(newMetadataLocation, metadata);
+        commitStatus =


Here we can use the original method from BaseMetastoreTableOperations.checkCommitStatus too.

…us instead of BaseMetastoreOperations.checkCommitStatus for Hive-Table.

nastra · 2024-03-22T10:42:17Z

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BaseMetastoreOperations {


Suggested change

public class BaseMetastoreOperations {

public abstract class BaseMetastoreOperations {

nastra · 2024-03-22T10:45:15Z

+  public void fullTableNameWithDifferentValues() {
+    String uriTypeCatalogName = "thrift://host:port/db.table";
+    String nameSpaceWithOneLevel = "ns";
+    String nameSpaceWithTwoLevel = "ns.l2";


Suggested change

String nameSpaceWithTwoLevel = "ns.l2";

String namespaceWithTwoLevels = "ns.l2";

nastra · 2024-03-22T10:45:31Z

+  @Test
+  public void fullTableNameWithDifferentValues() {
+    String uriTypeCatalogName = "thrift://host:port/db.table";
+    String nameSpaceWithOneLevel = "ns";


Suggested change

String nameSpaceWithOneLevel = "ns";

String namespace = "ns";

nastra · 2024-03-22T10:49:21Z

    try {
      Table table = clients.run(client -> client.getTable(fromDatabase, fromName));
-      HiveOperationsBase.validateTableIsIceberg(table, fullTableName(name, from));
+      validateTableIsIcebergTableOrView(contentType, name, table, from);


rather than passing the catalog name / table / from separately, it's probably better to just pass the table and the fullTableName: fullTableName(name, from)

nastra · 2024-03-22T10:49:33Z


+  private void validateTableIsIcebergTableOrView(
+      HiveOperationsBase.ContentType contentType,
+      String catalogName,


Suggested change

String catalogName,

nastra · 2024-03-22T10:49:47Z

+      HiveOperationsBase.ContentType contentType,
+      String catalogName,
+      Table table,
+      TableIdentifier identifier) {


Suggested change

TableIdentifier identifier) {

String fullTableName) {

nastra · 2024-03-22T11:01:51Z


  String table();

+  String catalogName();


I don't think we should be introducing catalogName()/ contentType() to this API just for printing it in a TRACE log inside loadHmsTable(). Please remove catalogName()/ contentType() from this API

nastra · 2024-03-22T11:02:40Z

+      return metaClients().run(client -> client.getTable(database(), table()));
+    } catch (NoSuchObjectException nte) {
+      LOG.trace(
+          "{} not found {}", contentType(), catalogName() + "." + database() + "." + table(), nte);


Suggested change

"{} not found {}", contentType(), catalogName() + "." + database() + "." + table(), nte);

"{} not found {}", contentType(), database() + "." + table(), nte);

this is just TRACE logging, so it should be ok to not have the catalog. I'm not even sure that logging stmt should even exist

nastra · 2024-03-22T11:11:50Z

+        Optional.ofNullable(tbl.getParameters()).orElseGet(Maps::newHashMap);
+
+    if (!obsoleteProps.contains(TableProperties.UUID) && uuid != null) {
+      parameters.put(TableProperties.UUID, uuid);


In https://github.com/apache/iceberg/pull/9852/files#diff-db46657b84d66e084e15f31b8dab21577efb2ae7102863f94c6c9477782de676R206 you're passing the View UUID and I wonder whether this is actually correct to do so?

This is another example where it can be dangerous to have a common method to set something that seems could be common across tables/views.

I would suggest to remove setCommonHmsParameters() from this API

nastra · 2024-03-22T11:13:29Z

    return maxHiveTablePropertySize() > 0;
  }

-  default void setSchema(TableMetadata metadata, Map<String, String> parameters) {


we need to keep the original method (for backwards compatibiliy), but the method can refer to the new setSchema() method:

default void setSchema(TableMetadata metadata, Map<String, String> parameters) { setSchema(metadata.schema(), parameters); }

nastra · 2024-03-22T11:14:10Z

    }
  }

-  static StorageDescriptor storageDescriptor(TableMetadata metadata, boolean hiveEngineEnabled) {


same as above, we need to keep StorageDescriptor storageDescriptor(TableMetadata metadata, boolean hiveEngineEnabled) but it can internally call static StorageDescriptor storageDescriptor(Schema schema, String location, boolean hiveEngineEnabled)

nastra · 2024-03-22T11:15:33Z

+  public void setHmsParameters(
+      TableMetadata metadata, Table tbl, String newMetadataLocation, Set<String> obsoleteProps) {

-  private void setHmsTableParameters(


please revert these changes in this method

nastra · 2024-03-22T11:17:50Z

  }

+  @Override
+  public String catalogName() {


please remove these two methods

nastra · 2024-03-22T11:32:35Z

        .doesNotContainKey(CURRENT_SNAPSHOT_TIMESTAMP);

-    ops.setSchema(metadata, parameters);
+    ops.setSchema(metadata.schema(), parameters);


Suggested change

ops.setSchema(metadata.schema(), parameters);

ops.setSchema(metadata, parameters);

no need to change this, as we need to keep the original method signature

…nd HiveOperationsBase.

Hive: Arrange common part of the code for Iceberg View.

438c413

github-actions Bot added core hive labels Mar 19, 2024

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/CatalogUtil.java Outdated

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HMSParametersSetter.java Outdated

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

nk1506 commented Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java Outdated

nk1506 commented Mar 19, 2024

View reviewed changes

nk1506 mentioned this pull request Mar 19, 2024

Hive: Add View support for HIVE catalog #9852

Merged

nastra reviewed Mar 19, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated

nk1506 marked this pull request as ready for review March 19, 2024 10:57

nastra reviewed Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 19, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 19, 2024

View reviewed changes

Addressed comments: removed unrelated changes with the parent scope.

d96572e

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

nastra reviewed Mar 21, 2024

View reviewed changes

nk1506 commented Mar 21, 2024

View reviewed changes

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

nk1506 commented Mar 21, 2024

View reviewed changes

Addressed comments: Removed abstraction from HiveOperationsBase for T…

ac26860

…able/View with other comments.

nk1506 force-pushed the hive_common_for_view branch from 90e8aaa to ac26860 Compare March 21, 2024 12:12

nk1506 commented Mar 21, 2024

View reviewed changes

Addressed comments: Used BaseMetastoreTableOperations.checkCommitStat…

f50f440

…us instead of BaseMetastoreOperations.checkCommitStatus for Hive-Table.

nk1506 requested a review from nastra March 22, 2024 10:01

nastra reviewed Mar 22, 2024

View reviewed changes

Addressed comments: reverted extra changes from HiveTableOperations a…

6bb5df5

…nd HiveOperationsBase.

nastra reviewed Mar 25, 2024

View reviewed changes

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

nastra approved these changes Mar 26, 2024

View reviewed changes

nastra merged commit 817a5e1 into apache:main Mar 26, 2024

sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024

Hive: Extract common code to be re-used for View support (apache#10001)

f2be7f2

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Hive: Extract common code to be re-used for View support (apache#10001)

5f372dc

lirui-apache mentioned this pull request Mar 31, 2025

Core, Hive: Double check commit status in case of commit conflict for NoLock #12637

Merged

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

Hive: Extract common code to be re-used for View support (apache#10001)

b817903

	* @return true if the new metadata location presents with current or previous metadata files.
	* @return true if the new metadata location is the current metadata location or present within previous metadata files.

	public class BaseMetastoreOperations {
	public abstract class BaseMetastoreOperations {

	String nameSpaceWithTwoLevel = "ns.l2";
	String namespaceWithTwoLevels = "ns.l2";

	String nameSpaceWithOneLevel = "ns";
	String namespace = "ns";

	"{} not found {}", contentType(), catalogName() + "." + database() + "." + table(), nte);
	"{} not found {}", contentType(), database() + "." + table(), nte);

	ops.setSchema(metadata.schema(), parameters);
	ops.setSchema(metadata, parameters);

Conversation

nk1506 commented Mar 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

nk1506 commented Mar 21, 2024

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nastra Mar 22, 2024 •

edited

Loading