Hive: Add View support for HIVE catalog #8907

nk1506 · 2023-10-23T13:12:23Z

Issues #8698

api/src/main/java/org/apache/iceberg/exceptions/NoSuchIcebergViewException.java

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

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

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

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

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

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

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

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java

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

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java

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

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

nk1506 · 2024-01-10T06:58:46Z

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

+      table = metaClients.run(client -> client.getTable(database, tableName));
+      HiveOperationsBase.validateTableOrViewIsIceberg(table, fullName);
+
+      if (table.getTableType().equalsIgnoreCase(TableType.VIRTUAL_VIEW.name())) {


If Hive Table is not iceberg table new metadataLocation should always be null.

IIRC the decision was to do not use the HMS tableType for this. Shouldn't we use BaseMetastoreTableOperations.TABLE_TYPE_PROP property instead?

I have segregated Iceberg table and Iceberg View check here.

pvary · 2024-01-10T13:59:41Z

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

+      if (!table.getTableType().equalsIgnoreCase(TableType.VIRTUAL_VIEW.name())) {
+        throw new NoSuchObjectException();
+      }


Why not throw the final exception immediately?

ajantha-bhat · 2024-01-11T10:19:02Z

@nk1506, @pvary: I have added this PR to upcoming 1.5.0 release.
I will also do one round of review tomorrow.

@nk1506: Please consolidate what comments are pending as now for this PR.
We can discuss what can be fixed in this PR and what can be done as a follow up.

nk1506 · 2024-01-11T10:28:28Z

@ajantha-bhat / @pvary I think only major comment left here is Should we use lock before committing view table or not ? If yes should we do with this PR ?

ajantha-bhat · 2024-01-11T11:39:53Z

@nk1506: Not just major, I want to conclude on all the comments that are open. So, we can move forward.

ajantha-bhat · 2024-01-18T09:14:58Z

I agree that we do need these improvements for views

purge support
Common interfaces for TableOperations + ViewOperations.
Common interfaces for TableMetadata + ViewMetadata.

But I don't think we should drag the view support for Hive catalog since REST, Nessie, JDBC already supports it.
Can we handle them in a follow up? I did see that we have opened the discussions on mailing list.

@pvary, @rdblue, @jbonofre, @nastra : What is your opinion on this?

nk1506 · 2024-01-18T14:33:38Z

I agree that we do need these improvements for views

purge support

Common interfaces for TableOperations + ViewOperations.

Common interfaces for TableMetadata + ViewMetadata.

But I don't think we should drag the view support for Hive catalog since REST, Nessie, JDBC already supports it. Can we handle them in a follow up? I did see that we have opened the discussions on mailing list.

@pvary, @rdblue, @jbonofre, @nastra : What is your opinion on this?

I have created an issue for abstracting out ViewMetadata and TableMetadata.

jbonofre · 2024-01-18T14:38:58Z

It makes sense to me to split in two parts:

First the view support on Hive catalog "as it is", to be consistent with other catalogs
Prepare a followup with the proposed improvements that could be leverage by all catalogs.

pvary · 2024-01-19T07:33:41Z

But I don't think we should drag the view support for Hive catalog since REST, Nessie, JDBC already supports it.

I am not sure that I would like to support a feature in production where manual cleanup is needed after deleting an object.
To be honest, I prefer not support something, until we have a technically robust implementation. Implementing views without locks would cause view history corruption, copy-pasting code will cause maintainability issues in the medium-long term. Just consider the decision, that we did not create a common ancestor for the metadata....
This kind of technical debt could be crippling for an OSS project, as everyone would wait for others to fix the accumulated issues.

jbonofre · 2024-01-19T08:47:20Z

@pvary I agree. Maybe I wasn't clear, my point was:

abstracting behaviors common to table and view makes sense but not a blocker for this PR imho
having something consistent about delete specifically to Hive (like locks, etc) is a requirement. I agree with you.

So, if you propose the later, I agree and it should be part of this PR. For the first point, not blocker for this PR and other PRs related to views.

pvary · 2024-01-19T09:09:05Z

abstracting behaviors common to table and view makes sense but not a blocker for this PR imho

If the community decides against abstracting out the common part of the Metadata, then we can decide how to handle the situation. On the other hand, until the discussion is ongoing, I would not run ahead with a hopefully temporary solution.

danielcweeks · 2024-01-20T21:46:28Z

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

-                .filter(
-                    table ->
-                        table.getParameters() != null
-                            && BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE


I'm concerned that this is changing behavior for the catalog. There was no filter on external table types previously and I don't believe that was actually required.

danielcweeks · 2024-01-20T23:29:28Z

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

+
+        // If we try to create the view but the metadata location is already set, then we had a
+        // concurrent commit
+        if (newView


This check doesn't look correct. If this is a newView there shouldn't be an existing view definition. What if this is an existing hive view that does not have a location set? It seems that this should fail if a view is found but you're attempting to create a new one.

As far as I can tell, I think this is borrowed from the HiveTableOperations::doCommit() code. There it makes a bit more sense as we get a lock, do a check, then fail if there's an existing table (before we write our table).

It seems the change to abstract out common logic is in https://github.com/apache/iceberg/pull/9461/files

I tried to follow the table behaviour . How concurrent commits will be different in case of View ? Am i missing something here?

danielcweeks · 2024-01-20T23:34:24Z

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

+        LOG.debug("Committing existing view: {}", fullName);
+      } else {
+        view =
+            newHmsTable(


When creating the table/view definition in hms, we should be setting something in the original and expanded text fields to indicate that this is an Iceberg view. This can help if something non-iceberg attempts to load the view definition.

Should we read this from metadata properties or add a constant ?

szehon-ho

Look forward to this pr. Looks like some of this is pending #9461, so will sense for me to look at that first

szehon-ho · 2024-01-23T00:51:41Z

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

@@ -284,7 +284,7 @@ private Map<String, String> tableOverrideProperties() {
    }
  }

-  protected static String fullTableName(String catalogName, TableIdentifier identifier) {
+  public static String fullTableName(String catalogName, TableIdentifier identifier) {


Looks a bit strange to have a util class in Base class, can we move it to some util (for example, CatalogUtil?)

szehon-ho · 2024-01-23T01:23:18Z

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

+
+        // If we try to create the view but the metadata location is already set, then we had a
+        // concurrent commit
+        if (newView


As far as I can tell, I think this is borrowed from the HiveTableOperations::doCommit() code. There it makes a bit more sense as we get a lock, do a check, then fail if there's an existing table (before we write our table).

It seems the change to abstract out common logic is in https://github.com/apache/iceberg/pull/9461/files

szehon-ho · 2024-01-23T01:46:54Z

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

-    if (currentMetadataLocation() != null && !currentMetadataLocation().isEmpty()) {
-      parameters.put(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation());
-    }
+    parameters.put(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE.toUpperCase(Locale.ENGLISH));


Shouldn't this should be view property?

I think in case of Hive-Table is should be always ICEBERG_TABLE_TYPE_VALUE = "iceberg";

nk1506 · 2024-04-04T17:09:42Z

Closing this PR as same has been addressed with ongoing PR .

nk1506 marked this pull request as draft October 23, 2023 13:12

github-actions bot added API hive labels Oct 23, 2023

nk1506 changed the title ~~Core: Add View support for HIVE catalog~~ Hive: Add View support for HIVE catalog Oct 25, 2023

nastra reviewed Oct 25, 2023

View reviewed changes

api/src/main/java/org/apache/iceberg/exceptions/NoSuchIcebergViewException.java Show resolved Hide resolved

nastra reviewed Oct 25, 2023

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_view branch from 0de09c2 to f7c6291 Compare October 26, 2023 05:12

github-actions bot added core build labels Oct 26, 2023

nk1506 marked this pull request as ready for review October 27, 2023 01:13

ajantha-bhat reviewed Oct 27, 2023

View reviewed changes

nk1506 force-pushed the hive_view branch 2 times, most recently from dc5d0c7 to b3c8edc Compare October 28, 2023 18:41

ajantha-bhat requested a review from pvary October 30, 2023 05:42

ajantha-bhat mentioned this pull request Oct 31, 2023

Nessie: Support views for NessieCatalog #8909

Merged

ajantha-bhat reviewed Oct 31, 2023

View reviewed changes

nastra reviewed Oct 31, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2023

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_view branch 3 times, most recently from 8625c9d to aaaa9a4 Compare November 2, 2023 09:45

nastra reviewed Nov 2, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java Outdated Show resolved Hide resolved

nastra reviewed Nov 2, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java Outdated Show resolved Hide resolved

nk1506 force-pushed the hive_view branch from aaaa9a4 to 9d59349 Compare November 2, 2023 17:08