HIVE-29461: Iceberg: HIVE_METASTORE_WAREHOUSE_EXTERNAL is ignored when initializing HiveCatalog by 1fanwang · Pull Request #6454 · apache/hive

1fanwang · 2026-04-27T08:30:39Z

What changes were proposed in this pull request?

Add HiveCatalog.EXTERNAL_WAREHOUSE_LOCATION ("external-warehouse") as a canonical catalog property and propagate it from HiveCatalog.initialize(name, properties) to hive.metastore.warehouse.external.dir, mirroring the existing handling for CatalogProperties.WAREHOUSE_LOCATION (including LocationUtil.stripTrailingSlash).

The two in-tree callers are unified onto the new constant:

HMSCatalogFactory#createCatalog drops its redundant configuration.set(...) workaround that was needed because the property was not being read from the properties map.
IcebergSummaryHandler#initialize switches from the unhyphenated "externalwarehouse" key (which was effectively dead code — HiveCatalog.initialize never read it) to the canonical name.

Why are the changes needed?

HiveCatalog.initialize(name, properties) propagated CatalogProperties.URI and CatalogProperties.WAREHOUSE_LOCATION to the Hadoop configuration but silently dropped any external-warehouse value, so getExternalWarehouseLocation() and convertToDatabase() failed with:

Warehouse location is not set: hive.metastore.warehouse.external.dir=null

whenever a caller reached HiveCatalog through the standard Iceberg Catalog.initialize(name, properties) API without separately mutating the Configuration first.

The two existing callers worked around the gap differently and used different property keys:

HMSCatalogFactory.java (line 86-91 before this change) put "external-warehouse" in the properties map AND re-set the value on the Configuration before initialize(), with a comment that explicitly acknowledged the API gap: // HiveCatalog reads this property directly from Configuration, not from properties map.
IcebergSummaryHandler.java (line 67 before this change) put "externalwarehouse" (no hyphen) in the properties map but did not perform the Configuration.set workaround — it only happened to work because the prior setConf(configuration) call carried the value through when MetastoreConf.WAREHOUSE_EXTERNAL was already set on the underlying Hadoop conf.

External integrations using the standard Iceberg Catalog.initialize(name, properties) API (e.g. Polaris) had no documented way to pass the external warehouse location and tripped the NPE. After this change the property is documented on HiveCatalog, propagated via initialize(), and the two key spellings are unified.

Does this PR introduce any user-facing change?

Yes — adds a new public catalog property external-warehouse (HiveCatalog.EXTERNAL_WAREHOUSE_LOCATION) that callers can pass into HiveCatalog.initialize(name, properties) to set the external warehouse location. Existing flows that set hive.metastore.warehouse.external.dir directly on the Configuration continue to work unchanged.

How was this patch tested?

Added testInitializeCatalogWithExternalWarehouseProperty in TestHiveCatalog, mirroring the existing testInitializeCatalogWithProperties. The test verifies that both WAREHOUSE_LOCATION and the new EXTERNAL_WAREHOUSE_LOCATION propagate to their respective Hadoop conf keys with trailing-slash stripping.

Local Maven test runs were blocked on unrelated lock contention from concurrent builds in my environment (maven-remote-resources-plugin: Could not acquire lock(s)), so I'm relying on CI for full verification. Happy to address any failures.

…n initializing HiveCatalog HiveCatalog.initialize() propagated CatalogProperties.WAREHOUSE_LOCATION to the Hadoop configuration but ignored the equivalent external-warehouse property, so getExternalWarehouseLocation() and convertToDatabase() failed with an NPE ("Warehouse location is not set: hive.metastore.warehouse.external.dir=null") whenever a caller reached HiveCatalog through the standard Iceberg Catalog.initialize(name, properties) API without separately mutating the Configuration. Callers worked around this either by re-setting the value on the Configuration before initialize() (HMSCatalogFactory) or by relying on a side-channel setConf() carrying the value through (IcebergSummaryHandler), and in both cases used a different, undocumented property key ("external-warehouse" vs "externalwarehouse"). Add HiveCatalog.EXTERNAL_WAREHOUSE_LOCATION ("external-warehouse") as the canonical catalog property and propagate it from initialize() to hive.metastore.warehouse.external.dir, mirroring the existing handling for WAREHOUSE_LOCATION (including LocationUtil.stripTrailingSlash). Update the two in-tree callers to use the constant and drop HMSCatalogFactory's redundant configuration.set() workaround. The unhyphenated key in IcebergSummaryHandler was effectively dead code (HiveCatalog never read it) and is replaced rather than retained. Adds testInitializeCatalogWithExternalWarehouseProperty mirroring testInitializeCatalogWithProperties to lock in the propagation behavior.

…actory deniskuzZ pointed out (PR review on HMSCatalogFactory.java) that HMSCatalogFactory.createCatalog already calls hiveCatalog.setConf(configuration) right before initialize(), and HiveCatalog reads WAREHOUSE_EXTERNAL straight from Configuration via getExternalWarehouseLocation(). The properties.put for external-warehouse was therefore redundant on this path even after the HiveCatalog#initialize fix; setConf already supplies the value. Removes the redundant block here. The HiveCatalog#initialize change still fixes the IcebergSummaryHandler path, which initializes by properties only without a setConf call.

1fanwang · 2026-04-27T12:13:58Z

@deniskuzZ Right — pushed c727def to drop the whole if (configExtWarehouse != null) block. setConf(configuration) here already gives HiveCatalog the value via getExternalWarehouseLocation() reading from Configuration. The HiveCatalog#initialize change still covers the IcebergSummaryHandler path, which initializes by properties only without a setConf call.

…s.WAREHOUSE_LOCATION deniskuzZ flagged on PR review that the external-warehouse properties.put shouldn't have been removed in the prior commit (c727def) -- the new HiveCatalog.EXTERNAL_WAREHOUSE_LOCATION constant is the contract HiveCatalog exposes for callers initializing by properties (IcebergSummaryHandler), and HMSCatalogFactory should keep using it for consistency with the other catalog properties set in the same block, even though setConf() on this specific path also supplies the value. Restores the if-block. Also applies their suggestion at the existing warehouse line: switch the literal "warehouse" to CatalogProperties.WAREHOUSE_LOCATION.

1fanwang · 2026-04-27T12:19:53Z

Sorry @deniskuzZ — misread your earlier "do we even need this?" as "remove it". Restored the external-warehouse properties.put in ce92efc, and applied your CatalogProperties.WAREHOUSE_LOCATION suggestion to the warehouse line in the same commit.

deniskuzZ · 2026-04-27T12:23:06Z

Sorry @deniskuzZ — misread your earlier "do we even need this?" as "remove it". Restored the external-warehouse properties.put in ce92efc, and applied your CatalogProperties.WAREHOUSE_LOCATION suggestion to the warehouse line in the same commit.

@1fanwang i meant do we need to call setConf at all, since we pass everything thought the properties?

1fanwang · 2026-04-27T12:24:32Z

Ahh sorry, second misread. I think setConf is still needed here because HMSCatalogFactory's configuration is the fully-populated HiveConf — HMS thrift auth, SSL/timeout, kerberos, the SERVLET_ID_KEY, etc. Without setConf, HiveCatalog falls back to its default Configuration and loses all of that. The new properties-only path makes sense for callers that initialize purely from a property map (IcebergSummaryHandler), but on this path setConf is providing more than the URI/warehouse/external-warehouse triple. Open to dropping it if there's a flow I'm missing — does HiveCatalog reconstruct the HMS-specific config from properties alone somewhere I haven't traced?

1fanwang · 2026-04-27T12:31:48Z

The one wrinkle on dropping setConf is CLIENT_POOL_CACHE_KEYS = "conf:SERVLET_ID_KEY" a few lines up — that tells HiveCatalog to read SERVLET_ID_KEY from Configuration when computing the per-test cache key, and without setConf HiveCatalog falls back to its default Configuration that doesn't have SERVLET_ID_KEY (so cache-key uniqueness breaks for the embedded tests). Leaning toward a separate follow-up JIRA for the setConf cleanup so this PR stays focused on HIVE-29461 — let me know if you'd rather roll it in.

deniskuzZ · 2026-04-27T12:35:41Z

The one wrinkle on dropping setConf is CLIENT_POOL_CACHE_KEYS = "conf:SERVLET_ID_KEY" a few lines up — that tells HiveCatalog to read SERVLET_ID_KEY from Configuration when computing the per-test cache key, and without setConf HiveCatalog falls back to its default Configuration that doesn't have SERVLET_ID_KEY (so cache-key uniqueness breaks for the embedded tests). Leaning toward a separate follow-up JIRA for the setConf cleanup so this PR stays focused on HIVE-29461 — let me know if you'd rather roll it in.

Should we introduce single constructor for that. Something similar to HadoopCatalog:
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java#L137-L140

1fanwang · 2026-04-27T12:36:35Z

Yeah, a HiveCatalog(Configuration, Map<String, String>) constructor mirroring HadoopCatalog L137-140 is the right shape. That's an iceberg-side change though, since HiveCatalog lives in iceberg-core. I can file the iceberg issue/PR for the constructor and a Hive follow-up JIRA for adopting it (rolling in the setConf cleanup at the same time), unless you're already tracking the iceberg side somewhere.

deniskuzZ · 2026-04-27T18:26:47Z

https://ci.hive.apache.org/job/hive-precommit/job/PR-6454/2/testReport/junit/org.apache.iceberg.metasummary/TestIcebergSummaryHandler/Testing___split_06___PostProcess___testInitialize/

1fanwang · 2026-04-27T19:57:59Z

https://ci.hive.apache.org/job/hive-precommit/job/PR-6454/2/testReport/junit/org.apache.iceberg.metasummary/TestIcebergSummaryHandler/Testing___split_06___PostProcess___testInitialize/

Thanks @deniskuzZ I was trying to check the exact failure however not able to view, does this require some ACL?

 Oops!

Not Found

This page may not exist, or you may not have permission to see it.

Not Found [Jenkins].pdf

1fanwang · 2026-04-27T20:17:42Z

Filed both:

iceberg #16134 — issue proposing HiveCatalog(Configuration, Map<String, String>) mirroring HadoopCatalog L137-140.
iceberg #16135 — implementation + test.

Hive follow-up JIRA for the HMSCatalogFactory adoption + the setConf cleanup will go up once the iceberg side lands. Keeping this PR focused on HIVE-29461.

@deniskuzZ

…iterals Address @deniskuzZ review on apache#6454: replace the literal 'uri' and 'warehouse' strings with CatalogProperties.URI and CatalogProperties.WAREHOUSE_LOCATION so the Iceberg-side property names stay consistent if they ever change upstream. CatalogProperties is already imported.

deniskuzZ

+1, pending tests

deniskuzZ · 2026-04-28T08:09:48Z

if you have Hive jira account - you should be able to view jenkins in read-only

currently it has just 1 test failure;

org.apache.iceberg.metasummary.TestIcebergSummaryHandler.testInitialize

java.lang.IllegalArgumentException: path must not be null or empty
	at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:141)
	at org.apache.iceberg.util.LocationUtil.stripTrailingSlash(LocationUtil.java:28)
	at org.apache.iceberg.hive.HiveCatalog.initialize(HiveCatalog.java:127)
	at org.apache.iceberg.metasummary.IcebergSummaryHandler.lambda$initialize$0(IcebergSummaryHandler.java:72)
	at org.apache.iceberg.metasummary.IcebergSummaryHandler.initialize(IcebergSummaryHandler.java:80)
	at org.apache.iceberg.metasummary.TestIcebergSummaryHandler.testInitialize(TestIcebergSummaryHandler.java:37)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

…dler MetastoreConf.WAREHOUSE_EXTERNAL and THRIFT_URIS default to "" (empty) when not configured, which the previous unconditional propertiesMap.put forwarded into HiveCatalog.initialize. After this PR's HiveCatalog change, the empty external-warehouse value reached LocationUtil.stripTrailingSlash and tripped IllegalArgumentException("path must not be null or empty"), breaking TestIcebergSummaryHandler.testInitialize. Mirror HMSCatalogFactory's pattern of only forwarding properties that have a non-empty value, and replace the remaining "warehouse"/"uri" string literals with CatalogProperties.WAREHOUSE_LOCATION / CatalogProperties.URI per review feedback.

1fanwang · 2026-04-29T17:37:09Z

https://ci.hive.apache.org/job/hive-precommit/job/PR-6454/2/testReport/junit/org.apache.iceberg.metasummary/TestIcebergSummaryHandler/Testing___split_06___PostProcess___testInitialize/

Root cause: MetastoreConf.WAREHOUSE_EXTERNAL and THRIFT_URIS default to "" (not null), so the containsKey checks in HiveCatalog.initialize passed and stripTrailingSlash("") tripped. Mirrored HMSCatalogFactory's skip-if-unset pattern (extended to empty) in IcebergSummaryHandler. Pushed in bb2f581.

1fanwang · 2026-04-30T08:07:43Z

looks like flaky ci tests failing

New failing - 4
Testing / split-05 / PostProcess / testSuccessIncrDumpMetrics – org.apache.hadoop.hive.ql.parse.repl.metric.TestReplicationMetricCollector
<1s
Testing / split-08 / PostProcess / testManualFailover – org.apache.hive.jdbc.TestActivePassiveHA
4s
Testing / split-08 / PostProcess / testHealthCheck – org.apache.hive.jdbc.TestActivePassiveHA
1m 0s
Testing / split-08 / PostProcess / testHealthCheckAuth – org.apache.hive.jdbc.TestActivePassiveHANew failing - 4
Testing / split-05 / PostProcess / testSuccessIncrDumpMetrics – org.apache.hadoop.hive.ql.parse.repl.metric.TestReplicationMetricCollector
<1s
Testing / split-08 / PostProcess / testManualFailover – org.apache.hive.jdbc.TestActivePassiveHA
4s
Testing / split-08 / PostProcess / testHealthCheck – org.apache.hive.jdbc.TestActivePassiveHA
1m 0s
Testing / split-08 / PostProcess / testHealthCheckAuth – org.apache.hive.jdbc.TestActivePassiveHA

https://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/detail/PR-6454/5/tests/

sonarqubecloud · 2026-04-30T14:02:38Z

Quality Gate passed

Issues
0 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

asf-ci-hive added the tests pending label Apr 27, 2026

1fanwang force-pushed the HIVE-29461-iceberg-external-warehouse branch from 78e0f09 to 9846386 Compare April 27, 2026 09:01

asf-ci-hive added tests failed and removed tests pending labels Apr 27, 2026

deniskuzZ reviewed Apr 27, 2026

View reviewed changes

Comment thread ...etastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java Outdated

deniskuzZ reviewed Apr 27, 2026

View reviewed changes

Comment thread ...etastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java Outdated

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending labels Apr 27, 2026

This was referenced Apr 27, 2026

Hive: add HiveCatalog(Configuration, Map<String, String>) convenience constructor apache/iceberg#16134

Open

Hive: add HiveCatalog(Configuration, Map<String, String>) convenience constructor apache/iceberg#16135

Closed

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Apr 27, 2026

deniskuzZ approved these changes Apr 28, 2026

View reviewed changes

deniskuzZ reviewed Apr 28, 2026

View reviewed changes

Comment thread iceberg/iceberg-handler/src/main/java/org/apache/iceberg/metasummary/IcebergSummaryHandler.java Outdated

asf-ci-hive added tests pending and removed tests unstable labels Apr 29, 2026

HIVE-29461: Drop redundant public modifier on test method (SonarQube)

5f5acce

asf-ci-hive added tests passed tests pending tests unstable and removed tests pending tests passed labels Apr 29, 2026

chore(ci): retrigger precommit

8d72d8a

asf-ci-hive added tests pending and removed tests unstable labels Apr 30, 2026

deniskuzZ reviewed Apr 30, 2026

View reviewed changes

Comment thread ...etastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java Outdated

HIVE-29461: Use StringUtils.isEmpty for config-string null/empty checks

b417680

asf-ci-hive added tests passed tests pending and removed tests pending tests passed labels Apr 30, 2026

asf-ci-hive added tests passed and removed tests pending labels Apr 30, 2026

deniskuzZ merged commit 83460e5 into apache:master Apr 30, 2026
4 checks passed

Uh oh!

Conversation

1fanwang commented Apr 27, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

deniskuzZ commented Apr 27, 2026

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

deniskuzZ commented Apr 27, 2026

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

deniskuzZ commented Apr 27, 2026

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

1fanwang commented Apr 27, 2026

Uh oh!

deniskuzZ left a comment

Choose a reason for hiding this comment

Uh oh!

deniskuzZ commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

1fanwang commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1fanwang commented Apr 30, 2026

Uh oh!

Uh oh!

sonarqubecloud Bot commented Apr 30, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deniskuzZ commented Apr 28, 2026 •

edited

Loading

1fanwang commented Apr 29, 2026 •

edited

Loading