HIVE-29578: Iceberg: add support for logical views

[difin]

What changes were proposed in this pull request?

Added support for Iceberg logical views in Hive for both HMS and REST catalogs.

Why are the changes needed?

To support Iceberg logical views. This can be especially useful for REST Catalog clients.

Does this PR introduce any user-facing change?

Yes, new HQL syntax:

1. A regular Hive view syntax with tblproperties ('view-format'='iceberg'):

create [or replace] view [if not exists] <NAME> tblproperties ('view-format'='iceberg') 
as select <COLUMNS> from <SRC_TABLE> where <CONDITION>;

2. Regular Hive syntax (without tblproperties ('view-format'='iceberg')) and Hive config hive.default.storage.handler.class pointing to a storage handler that supports external logical views:

set hive.default.storage.handler.class=org.apache.iceberg.mr.hive.HiveIcebergStorageHandler;

create [or replace] view [if not exists] <NAME> 
as select <COLUMNS> from <SRC_TABLE> where <CONDITION>;

How was this patch tested?

Created new and updated exiting unit and integration tests with Iceberg logical views test cases.

[InvisibleProgrammer]

I think it leads to inconsistent behavior: if we call the method on a table, it creates it if it is a new one and throws an AlreadyExistsException if the table already exists and also if a view with the same name is already exists.
But if we call this method on a view, it will create it even if a table exists with the same name and replaces the existing one if the view is already exists.

[difin]

This method cannot be called on a view if it already exists - see #6449 (comment)

[InvisibleProgrammer]

The view text is pretty interesting difference between Hive and Iceberg:
Hive stores it in two versions: original and expanded text.

Iceberg has only one field, sql.

I think if we want to be compatible with both Hive and Iceberg, I would rather choose the original text here and add the expanded text as a tableproperty.

But also, there is a catch: the sql is stored in ViewVersion so it can differ for each version. But in Hive, it is a single property of the table metadata.

Honestly, I'm not sure what is the correct solution to do that but if we store only one of them, I fear we can loose functionality on Hive side.

[difin]

I believe passing table.getViewExpandedText() is more correct because SemanticAnalyzer uses expanded text when resolving views.

On the write view path, view text is stored in HMS mainly for passing it to the IcebergNativeLogicalViewSupport.createOrReplaceNativeView, but on the read path view text is always retrieved from Iceberg API both on HMS and REST paths in BaseHiveIcebergMetaHook:

  @Override
  public void postGetTable(org.apache.hadoop.hive.metastore.api.Table hmsTable) {
    if (hmsTable != null) {
      try {
        if (isNativeIcebergLogicalView(hmsTable)) {
          IcebergNativeLogicalViewSupport.enrichHmsTableFromIcebergView(hmsTable, conf);
          return;
        }

[kasakrisz]

I agree on storing the expanded query text if only one field is available. The expanded query text is used by the Hive compiler to resolve view definitions. IIUC the original query text is more user friendly when printing the view definition via describe.

[InvisibleProgrammer]

I would add the original text as a query property to be able to support the same kind of output. But I can live with this solution as well.

[kasakrisz]

Created https://issues.apache.org/jira/browse/HIVE-29655 to track this.

[InvisibleProgrammer]

I'm pretty sure I'm missing something: what is the reason of checking if the catalog is Closeable and ignoring it?

[difin]

buildIcebergCatalog gives us a one-off catalog for this call. The Catalog interface isn’t Closeable, but some implementations are (RestCatalog is Closable) and they hold clients/IO that need to be closed when we’re done. HiveCatalog isn’t, so we only wrap when instanceof Closeable.

We are not ignoring it. The try (Closeable ignored = closeable) bit is just try-with-resources, we need a variable so close() runs at the end of the block. We don’t reference it in the body because we already use catalog for loadView. Naming it as ignored is only to signal “this exists for cleanup, not for logic.” Without it you can leak connections on every postGetTable enrich against REST.

[InvisibleProgrammer]

You're right and I'm stupid :D

Still, I think this way of writing hurts the readability: CatalogUtil.buildIcebergCatalog.
What's about calling loadAndApplyView and if the catalog is closeable, close it? It would flatten the code.

try {
  if (catalog instanceof Closeable closeable) {
    try (Closeable ignored = closeable) {
      loadAndApplyView(hmsTable, conf, catalog, catalogName, identifier);
    }
  } else {
    loadAndApplyView(hmsTable, conf, catalog, catalogName, identifier);
  }
} catch (IOException e) {
  throw new UncheckedIOException("Failed to close Iceberg catalog", e);
}
...
try {
  loadAndApplyView(hmsTable, conf, catalog, catalogName, identifier);
}
catch (IOException e) {
      throw new UncheckedIOException("Failed to close Iceberg catalog", e);
}
finally {
  if (catalog instanceof Closeable closeable) {
    catalog.close();
  }
}

[difin]

This doesn't compile because catalog.close() method does not exist.
Also, loadAndApplyView does not throw IOException, only close() does, and that runs in finally after the catch. So that catch never handles close errors.

I agree that the original way of writing hurts readability.
Fixed by using a helper method.

[InvisibleProgrammer]

In Iceberg view, timestamp-ms in the view version stores the create time.

[difin]

Changed to get it from the Iceberg metadata instead.

[InvisibleProgrammer]

For tables, we check if the table is already existing. Why don't we need a similar check in case of views?

[difin]

Same as #6449 (comment)

[InvisibleProgrammer]

I'm not sure if this code is defensive enough: even if the code currently can only run on a view it is not exists, those checks is pretty far from the execution, not even in the same module.
A future modification can easily break this check and there is no way the author of that future change will know about this execution path.

[difin]

I agree that writing defensive code is good, but there is a trade off with performance.
I didn't add a check here that the view exists because it is checked before in the execution path, although in a different module.

@kasakrisz, what is your opinion on this?

[kasakrisz]

The view's existence is checked here:

hive/ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java

Lines 49 to 50 in 9f387af

	Table oldview = context.getDb().getTable(desc.getViewName(), false);
	if (oldview != null) {

• Both CreateViewOperation.execute and the methods in BaseHiveIcebergMetaHook run on the client side. If we want to add extra checks, I think they should be added on the server side.
• To add extra checks to the server side, we will need to either extend the CreateTableRequest API with fields like replace and ifNotExists, or create a brand new API for creating views. This introduces an API change either way, so all types of views should be addressed. Since this isn't currently supported for native Hive views, we can handle it in a follow-up patch.
• Performance for a CREATE VIEW statement is not a high priority because it is not a frequently executed operation.

[InvisibleProgrammer]

Because of the hook based architecture of the Hive metadata handling, I interpret BaseHiveIcebergMetaHook as one of the entry points for the Iceberg code. To me, it is a clear module boundary.
Even if I disagree checking the existence only on a lower level of the code execution I can live with it.

[InvisibleProgrammer]

Is it a timestamp or some remaining time? I wonder if it would worth masking out those values or not to avoid test flakyness.

[difin]

2147483647 is Integer.MAX_VALUE - the HMS Table.retention field is set in MetastoreUtil.applyIcebergViewToHmsTable.

[difin]

I removed setting retention to match HMS behavior which sets 0 by default.

[InvisibleProgrammer]

On conceptional level, I have two major questions:
Should the Rest client handle views as tables? Or views should be views? At the current implementation it mixes them and basic CRUD operations doesn't separate them. For example, listing tables can receive both tables and views. But we don't know if a given entry is a table or a view.

The other important question is the boundary between what we store on HMS and what we store on Iceberg side?
What happens if we create a native Iceberg view? Do we store the metadata only on Iceberg side? Or duplicate it between Hive and Iceberg? What if we duplicate them? How we can make sure the duplicated data is in sync? And what is the plan if it is not in sync? For example, I'm creating a native Iceberg view in Beeline. Connect to the Rest client. Modify the view. Go back to Hive and query this view. What will happen?

[difin]

@difin When compiling a query referencing an Iceberg logical views the view definition query should come via Iceberg api from the Iceberg metadata files not from the HMS backend db.

In HiveRESTCatalogClient.java I found the implementation of this logic

if (restCatalog instanceof ViewCatalog viewCatalog) {
        try {
          View icebergView = viewCatalog.loadView(id);
          return MetastoreUtil.toHiveView(icebergView, conf);
        } catch (NoSuchViewException viewMissing) {
          throw new NoSuchObjectException();
        }

Maybe I'm missing something but haven't found the implementation of this in case of HMS catalog via thrift. Could you please point it out?

Done - previously Hive was getting the view query definition that was stored in HMS, now I made it retrieve view definition from Iceberg files (via Iceberg API) in BaseHiveIcebergMetaHook.postGetTable:

  public void postGetTable(org.apache.hadoop.hive.metastore.api.Table hmsTable) {
    if (hmsTable != null) {
      try {
        if (isNativeIcebergLogicalView(hmsTable)) {
          if (!usesRestMetastoreClient(conf)) {
            IcebergNativeLogicalViewSupport.enrichHmsTableFromIcebergView(hmsTable, conf);
          }
          return;
        }

[difin]

On conceptional level, I have two major questions: Should the Rest client handle views as tables? Or views should be views? At the current implementation it mixes them and basic CRUD operations doesn't separate them. For example, listing tables can receive both tables and views. But we don't know if a given entry is a table or a view.

Same as #6449 (comment), answered there.

The other important question is the boundary between what we store on HMS and what we store on Iceberg side? What happens if we create a native Iceberg view? Do we store the metadata only on Iceberg side? Or duplicate it between Hive and Iceberg? What if we duplicate them? How we can make sure the duplicated data is in sync? And what is the plan if it is not in sync? For example, I'm creating a native Iceberg view in Beeline. Connect to the Rest client. Modify the view. Go back to Hive and query this view. What will happen?

HMS stores only a view identity; at the read time, we construct HMS in-memory view object that gets enriched from the Iceberg metadata (including View SQL text) on both HMS and REST client paths in BaseHiveIcebergMetaHook:

  @Override
  public void postGetTable(org.apache.hadoop.hive.metastore.api.Table hmsTable) {
    if (hmsTable != null) {
      try {
        if (isNativeIcebergLogicalView(hmsTable)) {
          IcebergLogicalViewSupport.enrichHmsTableFromIcebergView(hmsTable, conf);
          return;
        }

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud