You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "ayushtkn (via GitHub)" <gi...@apache.org> on 2026/07/03 22:11:04 UTC
[PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
ayushtkn opened a new pull request, #4966:
URL: https://github.com/apache/polaris/pull/4966
Fixes: https://github.com/apache/polaris/pull/4963#discussion_r3521094048
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4966:
URL: https://github.com/apache/polaris/pull/4966#issuecomment-4920283691
Per `dev` discussion (link above) it looks like we have consensus to merge this PR.
I'll merge tomorrow if there are no fresh comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #4966:
URL: https://github.com/apache/polaris/pull/4966
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4966:
URL: https://github.com/apache/polaris/pull/4966#issuecomment-4909127614
Since it's a user-visible config change, let's keep the PR open for one more day in case other reviewer want to comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3533756593
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1316,15 +1306,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() {
final String anotherTableLocation =
String.format("s3://my-bucket/path/to/data/another_table_%s/", tableSuffix);
- metaStoreManager.updateEntityPropertiesIfNotChanged(
- polarisContext,
- List.of(PolarisEntity.toCore(catalogEntity)),
- new CatalogEntity.Builder(CatalogEntity.of(catalogEntity))
- .addProperty(
- FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false")
- .addProperty(
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
- .build());
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
Review Comment:
> Same: setting ALLOW_EXTERNAL_METADATA_FILE_LOCATION does not see to be necessary here. The test passed without it in my env. ๐ค
There is no `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` here ๐ค
Moreover this is just a refactor, it was already there but not calling the available `updateCatalogProperties`, So, I just refactored the existing test to use that as I was touching this file. Exact same configs, just the method.
>I wonder about other cases of putting ALLOW_EXTERNAL_METADATA_FILE_LOCATION into catalog properties. The settings were present before, but because they were not loaded from catalog properties before this PR, they are probably useless... WDYT?
All these configs have a default `false` which is the most relevant case. In general can't say for all, but mostly we too disable for even HiveCatalog in Iceberg to have any file outside the Table directory, so, the default `false` is the most idle case, so I believe most of the people won't tweak all these configs ever unless there is some specific use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3533756593
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1316,15 +1306,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() {
final String anotherTableLocation =
String.format("s3://my-bucket/path/to/data/another_table_%s/", tableSuffix);
- metaStoreManager.updateEntityPropertiesIfNotChanged(
- polarisContext,
- List.of(PolarisEntity.toCore(catalogEntity)),
- new CatalogEntity.Builder(CatalogEntity.of(catalogEntity))
- .addProperty(
- FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false")
- .addProperty(
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
- .build());
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
Review Comment:
> Same: setting ALLOW_EXTERNAL_METADATA_FILE_LOCATION does not see to be necessary here. The test passed without it in my env. ๐ค
There is no `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` here ๐ค. Am I reading the wrong lines?
These two were already there but not calling the available `updateCatalogProperties` but direclty doing ``metaStoreManager.updateEntityPropertiesIfNotChanged(
polarisContext,``, So, I just picked the exact same configs, just the method. Let me know If I am catching it wrong
>I wonder about other cases of putting ALLOW_EXTERNAL_METADATA_FILE_LOCATION into catalog properties. The settings were present before, but because they were not loaded from catalog properties before this PR, they are probably useless... WDYT?
All these configs have a default `false` which is the most relevant case. In general can't say for all, but mostly we too disable for even HiveCatalog in Iceberg to have any file outside the Table directory, so, the default `false` is the most idle case, so I believe most of the people won't tweak all these configs ever unless there is some specific use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #4966:
URL: https://github.com/apache/polaris/pull/4966#issuecomment-4910038369
Thanks @ayushtkn for working on it. Maybe we could deprecate `ALLOW_EXTERNAL_TABLE_LOCATION`, instead of keeping patching it. I opened a dev mailing list discussion here, https://lists.apache.org/thread/t2pn7t2h983qjpkzq7n08cxxv941n8pm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3521998459
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -305,6 +305,8 @@ public static void enforceFeatureEnabledOrThrow(
public static final FeatureConfiguration<Boolean> ALLOW_EXTERNAL_METADATA_FILE_LOCATION =
PolarisConfiguration.<Boolean>builder()
.key("ALLOW_EXTERNAL_METADATA_FILE_LOCATION")
+ .catalogConfig("polaris.config.allow.external.metadata.file.location")
+ .legacyCatalogConfig("allow.external.metadata.file.location")
.description(
Review Comment:
This change makes ALLOW_EXTERNAL_METADATA_FILE_LOCATION overridable via catalog properties, but the feature-flag documentation currently lacks a corresponding โCatalog Configโ entry for this flag (unlike similar flags). Please check whether the config-doc generation (or the docs source) needs to be regenerated/updated so the new catalog-level keys are documented (e.g., site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3532869578
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1316,15 +1306,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() {
final String anotherTableLocation =
String.format("s3://my-bucket/path/to/data/another_table_%s/", tableSuffix);
- metaStoreManager.updateEntityPropertiesIfNotChanged(
- polarisContext,
- List.of(PolarisEntity.toCore(catalogEntity)),
- new CatalogEntity.Builder(CatalogEntity.of(catalogEntity))
- .addProperty(
- FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false")
- .addProperty(
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
- .build());
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
Review Comment:
Same: setting `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` does not see to be necessary here. The test passed without it in my env. ๐ค
I wonder about other cases of putting `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` into catalog properties. The settings were present before, but because they were not loaded from catalog properties before this PR, they are probably useless... WDYT?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -2790,7 +2775,8 @@ public void testUpdatePropertiesRejectsOutOfTableWriteMetadataLocation() {
updateCatalogProperties(
Map.of(
FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true",
+ FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION.catalogConfig(), "false"));
Review Comment:
Setting `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` does not see to be necessary here. The test passed without it in my env. ๐ค
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -305,6 +305,8 @@ public static void enforceFeatureEnabledOrThrow(
public static final FeatureConfiguration<Boolean> ALLOW_EXTERNAL_METADATA_FILE_LOCATION =
PolarisConfiguration.<Boolean>builder()
.key("ALLOW_EXTERNAL_METADATA_FILE_LOCATION")
+ .catalogConfig("polaris.config.allow.external.metadata.file.location")
+ .legacyCatalogConfig("allow.external.metadata.file.location")
Review Comment:
"legacy" names are probably not necessary for new properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3539711767
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1316,15 +1306,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() {
final String anotherTableLocation =
String.format("s3://my-bucket/path/to/data/another_table_%s/", tableSuffix);
- metaStoreManager.updateEntityPropertiesIfNotChanged(
- polarisContext,
- List.of(PolarisEntity.toCore(catalogEntity)),
- new CatalogEntity.Builder(CatalogEntity.of(catalogEntity))
- .addProperty(
- FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false")
- .addProperty(
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
- .build());
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
Review Comment:
However, since it's not affected by the main change in this PR, let's keep it (maybe remove later).
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -2790,7 +2775,8 @@ public void testUpdatePropertiesRejectsOutOfTableWriteMetadataLocation() {
updateCatalogProperties(
Map.of(
FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true",
+ FeatureConfiguration.ALLOW_EXTERNAL_METADATA_FILE_LOCATION.catalogConfig(), "false"));
Review Comment:
Setting `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` does not seem to be necessary here. The test passed without it in my env. ๐ค
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4966:
URL: https://github.com/apache/polaris/pull/4966#discussion_r3539707501
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1316,15 +1306,10 @@ public void testUpdateNotificationCreateTableInExternalLocation() {
final String anotherTableLocation =
String.format("s3://my-bucket/path/to/data/another_table_%s/", tableSuffix);
- metaStoreManager.updateEntityPropertiesIfNotChanged(
- polarisContext,
- List.of(PolarisEntity.toCore(catalogEntity)),
- new CatalogEntity.Builder(CatalogEntity.of(catalogEntity))
- .addProperty(
- FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false")
- .addProperty(
- FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true")
- .build());
+ updateCatalogProperties(
+ Map.of(
+ FeatureConfiguration.ALLOW_EXTERNAL_TABLE_LOCATION.catalogConfig(), "false",
+ FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION.catalogConfig(), "true"));
Review Comment:
Sorry, I meant `ALLOW_UNSTRUCTURED_TABLE_LOCATION` in this case :facepalm:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org