Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. #4011

SinghAsDev · 2022-01-30T20:30:16Z

Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. This resolves #3994

SinghAsDev · 2022-01-30T21:32:12Z

@jackye1995 @rdblue @anuragmantri can you help review this, thanks

api/src/main/java/org/apache/iceberg/catalog/Catalog.java

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

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

rdblue · 2022-01-30T23:32:40Z

I took a quick look. I think the idea is reasonable, but the implementation needs to be changed:

I think table-default and table-override are better property prefixes
This should not modify the Catalog interface or CachingCatalog
The best place to add this is in BaseMetastoreCatalog or other catalogs you need to update
Subclasses should pass properties using super(...) in the constructor, not set parent fields
Please reduce the footprint of this change by not modifying classes unnecessarily; add defaults to avoid touching so many files

…el using catalog properties.

SinghAsDev · 2022-01-31T00:57:04Z

@rdblue thanks for the review, updated to keep changes to hive and hadoop catalogs.

rdblue · 2022-01-31T16:23:37Z

Thanks for the quick turn-around @SinghAsDev! I'll take another look, hopefully today.

SinghAsDev · 2022-02-04T15:10:46Z

Hi @rdblue just wanted to bump this and see if you would have cycles to review this. Thanks!

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

rajarshisarkar · 2022-05-09T11:30:58Z

@SinghAsDev As discussed over Slack, this feature would benefit many EMR cluster users. I wanted to check with you if we can fast track this PR. In case you are keeping busy, can I collaborate with you to close this PR (as a co-author)?

SinghAsDev · 2022-05-09T17:35:30Z

Hey @rajarshisarkar so sorry for not being able to spend time on this 😞. I think if you have time I would appreciate the help to help close this, thanks. If you don't get time either, I can try to get to it by end of this week.

rajarshisarkar · 2022-05-10T06:19:41Z

Hi @SinghAsDev, I am happy to take this PR to closure. Can you please give access to your fork so that I can push new commits in your branch.

rajarshisarkar · 2022-05-11T09:43:03Z

Thanks, @SinghAsDev for providing the access! I will resolve the pending review comments.

rajarshisarkar · 2022-05-11T12:57:10Z

Ping @rdblue @kbendick @singhpk234 for review.

singhpk234 · 2022-05-12T05:32:19Z

aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java

+  public void testTablePropsDefinedAtCatalogLevel() {
+    String namespace = createNamespace();
+    String tableName = getRandomName();
+    TableIdentifier tableIdent = TableIdentifier.of(namespace, tableName);
+    ImmutableMap<String, String> catalogProps = ImmutableMap.of(
+        "table-default.key1", "catalog-default-key1",
+        "table-default.key2", "catalog-default-key2",
+        "table-default.key3", "catalog-default-key3",
+        "table-override.key3", "catalog-override-key3",
+        "table-override.key4", "catalog-override-key4",
+        "warehouse", "s3://" + testBucketName + "/" + testPathPrefix);
+
+    glueCatalog.initialize("glue", catalogProps);
+
+    Schema schema = new Schema(
+        NestedField.required(3, "id", Types.IntegerType.get(), "unique ID"),
+        NestedField.required(4, "data", Types.StringType.get())
+    );
+
+    org.apache.iceberg.Table table = glueCatalog.buildTable(tableIdent, schema)
+        .withProperty("key2", "table-key2")
+        .withProperty("key3", "table-key3")
+        .withProperty("key5", "table-key5")
+        .create();
+
+    Assert.assertEquals(
+        "Table defaults set for the catalog must be added to the table properties.",
+        "catalog-default-key1",
+        table.properties().get("key1"));
+    Assert.assertEquals(
+        "Table property must override table default properties set at catalog level.",
+        "table-key2",
+        table.properties().get("key2"));
+    Assert.assertEquals(
+        "Table property override set at catalog level must override table default" +
+            " properties set at catalog level and table property specified.",
+        "catalog-override-key3",
+        table.properties().get("key3"));
+    Assert.assertEquals(
+        "Table override not in table props or defaults should be added to table properties",
+        "catalog-override-key4",
+        table.properties().get("key4"));
+    Assert.assertEquals(
+        "Table properties without any catalog level default or override should be added to table" +
+            " properties.",
+        "table-key5",
+        table.properties().get("key5"));
+  }


should we move this test to TestGlueCatalog instead, as at present we don't run integ test as part of our GA. WDYT ?

Currently in TestGlueCatalog we mock objects of GlueCatalog and GlueClient. The class is mainly used for creating/listing/dropping/renaming namespace/tables by mocking the response. Also, it looks like we cannot mock S3 Client in S3FileIO directly from TestGlueCatalog. Do let me know your thoughts.

I personally really don't like working with mocks, so I understand what you mean. I would consider adding some test that does get run through the CI test suite, to ensure that any changes that are made in the API are made against the Glue catalog (outside of compiler related errors).

As it is true that the AWS integration tests aren't run on any schedule within the open source community (that I'm aware of) due to lack of infra.

That said, I wouldn't consider it a hard requirement given that HadoopCatalog and HiveCatalog both have tests. Maybe adding GlueCatalog to the existing CatalogTests suite would be beneficial in the long run? Something to consider.

I personally really don't like working with mocks, so I understand what you mean

Yes, one more limitation here with mocking the glue client is we cannot mock the glue table response with a path for the BaseMetastoreTableOperations.METADATA_LOCATION_PROP property (as it starts checking for the existence of the file while refreshing the metadata in BaseMetastoreTableOperations).

I would consider adding some test that does get run through the CI test suite, to ensure that any changes that are made in the API are made against the Glue catalog

I have added a test which asserts that the properties API should not return an empty map from Glue Catalog when the catalog properties are initialised. The processing of the properties in BaseMetastoreCatalog is validated by HadoopCatalog and HiveCatalog. Meanwhile, TestGlueCatalogTable should check the actual behaviour for GlueCatalog.

Maybe adding GlueCatalog to the existing CatalogTests suite would be beneficial in the long run? Something to consider.

Yes, I think adding GlueCatalog to the existing CatalogTests suite would be beneficial. I can publish a separate PR for the same.

Please let me know your thoughts.

Yes, one more limitation here with mocking the glue client is we cannot mock the glue table response with a path for the BaseMetastoreTableOperations.METADATA_LOCATION_PROP property (as it starts checking for the existence of the file while refreshing the metadata in BaseMetastoreTableOperations)

+1 understood. Wondering if we should comment this somewhere. I suppose a standalone comment on this PR is sufficient (or maybe in the final squashed git PR summarty a small mention of that in the extended message?). But overall +1 understood. Mocks can't solve everything.

I have added a test which asserts that the properties API should not return an empty map from Glue Catalog when the catalog properties are initialised. The processing of the properties in BaseMetastoreCatalog is validated by HadoopCatalog and HiveCatalog. Meanwhile, TestGlueCatalogTable should check the actual behaviour for GlueCatalog.

+1

Yes, I think adding GlueCatalog to the existing CatalogTests suite would be beneficial. I can publish a separate PR for the same.

Major +1. That would be really beneficial. Even if it involves some mocks (right now there's backing catalog, which for REST we use JDBC via the CatalogAdaptor.

Even just having some mocks there one time would make it easy for people who aren't super knowledgable in Glue to add tests that hit multiple catalogs without knowing the details of all of them (to some degree or with review).

Definitely for a separate PR though =)

rajarshisarkar · 2022-05-16T09:54:06Z

Ping @rdblue @kbendick for review.

kbendick · 2022-05-17T17:35:31Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

  private LockManager lockManager;
  private CloseableGroup closeableGroup;
-  private Map<String, String> catalogProperties;
+  private Map<String, String> catalogProperties = Collections.emptyMap();


Nit: Unless we need null values in the map, we almost always prefer shaded-guava's ImmutableMap.of().

kbendick

Thanks @SinghAsDev for pinging me again!

Left some more comments. I will do more review shortly, but I have some open-ended questions:

Do we have any worries about catalog defaults accidentally affecting a table that a user didn't intend it to? I know some properties have been called out as possibly better suited as per-job definitions (like write.upsert.enabled for Flink Iceberg Sink).

This might be a non-concern, particularly if it's well documented, but we might want to at least log the property keys for which we pulled a default value from the catalog level?

Would love to hear from others if they have any similar concerns, and if there's any proposed solution.

In the REST catalog, we do allow for a catalog-level override via the /config endpoint, so there's definitely some level of precedence, but this covers a bit more surface area than that.

kbendick · 2022-05-17T17:36:33Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java


  @Override
  public void initialize(String name, Map<String, String> properties) {
+    this.catalogProperties = properties;


Nit: Might want to wrapt his in ImmutableMap.copyOf(properties) (which is a no-op if the underlying map is an ImmutableMap. Though again if you need null-values then that won't work for you.

kbendick · 2022-05-17T17:42:20Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java


+  @VisibleForTesting
+  void initialize(String name, String path, AwsProperties properties, GlueClient client,
+      LockManager lock, FileIO io, Map<String, String> catalogProps) {


Nit / non-blocking: The indentation for this seems a little weird to me.

If all of the properties are on the next line, will it fit? I do see conflicting things at times so it's not a huge deal, but I believe the preferred way would be something more like:

@VisibleForTesting void initialize(String name, String path, AwsProperties properties, GlueClient client, LockManager lock, FileIO io, Map<String, String> catalogProps) {

But again, I see different formats so without clarification, I'd either go with the above, a single line (the next line) if they all fit, or just leave it unless somebody states otherwise. I'll follow up for more clarification, but this is absolutley non-blocking.

Agreed. The starting indent of all argument lines should match. That could be starting after initialize( or using a continuation indent on the following line. But the important thing is that all lines with arguments indent arguments at the same place.

kbendick · 2022-05-17T17:44:10Z

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

  }

+  protected Map<String, String> properties() {
+    return Collections.emptyMap();


Nit: If it's empty, usually we use ImmutableMap.of(). I see you updated from ImmutableMap.builder so I assume you need null values, but I think even still we'd normally use ImmutableMap.of() in the empty case.

kbendick · 2022-05-17T17:45:06Z

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

    private final TableIdentifier identifier;
    private final Schema schema;
-    private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+    private final Map<String, String> tableProperties = Maps.newHashMap();


Is this needed for null-values? If so, can we ensure that we have tests that include null values? Otherwise it's likely to get refactored at some point.

I don't think we will need null values.

It's mainly because the map gets altered with tableProperties.putAll(tableDefaultProperties()); and tableProperties.putAll(tableOverrideProperties());

kbendick · 2022-05-17T17:48:38Z

core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

  private FileIO fileIO;
  private LockManager lockManager;
  private boolean suppressPermissionError = false;
+  private Map<String, String> catalogProperties = Collections.emptyMap();


Nit / non-blocking : Does this need to be initialized? I'm assuming this is to suppress potential not-initialized errors?

kbendick · 2022-05-17T17:50:18Z

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

+  public static final String TABLE_DEFAULT_PREFIX = "table-default.";
+  public static final String TABLE_OVERRIDE_PREFIX = "table-override.";


Nit / open-question: How do people feel about these names? I'd kind of like them to have properties or something in them, but this might be overkill.

I'm good with this.

kbendick · 2022-05-17T17:51:46Z

core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java


+  @Override
+  protected Map<String, String> properties() {
+    return catalogProperties;


Nit: For here, _if it will remove the not-initialized warning from catalogProperties, we often do the following:

return catalogProperies != null ? ImmutableMap.of() : catalogProperties;

rdblue · 2022-05-17T22:46:31Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

  }

+  @VisibleForTesting
+  void initialize(String name, String path, AwsProperties properties, GlueClient client,


It isn't clear why this method is needed. @SinghAsDev, is this necessary?

This shouldn't be necessary, it is just used in test to check properties map is not empty when catalog properties is initialised.

rdblue · 2022-05-17T22:51:53Z

Since all that was left were nits, I went ahead and merged this. Thanks for the contribution, @SinghAsDev!

rajarshisarkar · 2022-05-18T04:31:29Z

Thanks, @SinghAsDev for the contribution - this is a really important feature for the EMR customers.
Thanks, @rdblue and @kbendick for the reviews!

I'll shall address the nitpicks and the GlueCatalog test improvements in a follow-up PR.

rajarshisarkar · 2022-05-18T10:28:00Z

Do we have any worries about catalog defaults accidentally affecting a table that a user didn't intend it to? I know some properties have been called out as possibly better suited as per-job definitions (like write.upsert.enabled for Flink Iceberg Sink).

This might be a non-concern, particularly if it's well documented, but we might want to at least log the property keys for which we pulled a default value from the catalog level?

Would love to hear from others if they have any similar concerns, and if there's any proposed solution.

@kbendick I don't think we need to worry as only the catalog properties with a certain prefix are considered, +1 for logging.

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

Lines 226 to 233 in 13b1a73

    
               /** 
        
                * Get default table properties set at Catalog level through catalog properties. 
        
                * 
        
                * @return default table properties specified in catalog properties 
        
                */ 
        
               private Map<String, String> tableDefaultProperties() { 
        
                 return PropertyUtil.propertiesWithPrefix(properties(), CatalogProperties.TABLE_DEFAULT_PREFIX); 
        
               }

github-actions bot added API AWS core hive NESSIE spark labels Jan 30, 2022

rdblue reviewed Jan 30, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/catalog/Catalog.java Outdated Show resolved Hide resolved

rdblue reviewed Jan 30, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/catalog/Catalog.java Outdated Show resolved Hide resolved

rdblue reviewed Jan 30, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/catalog/Catalog.java Outdated Show resolved Hide resolved

rdblue reviewed Jan 30, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Jan 30, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/CachingCatalog.java Outdated Show resolved Hide resolved

Allow table defaults to be configured and/ or enforced at catalog lev…

add90e0

…el using catalog properties.

SinghAsDev force-pushed the tbl_defaults branch from 5c10b26 to add90e0 Compare January 31, 2022 00:48

Make catalogProps field private

4d26bb3

kbendick reviewed Feb 4, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

szehon-ho mentioned this pull request Feb 5, 2022

Disable metrics for very wide tables to reduce metadata size #3959

Merged

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

rdblue reviewed Feb 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

Updates

7eb19c5

SinghAsDev commented Feb 9, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java Outdated Show resolved Hide resolved

Minor cleanup

46711e0

singhpk234 mentioned this pull request Apr 19, 2022

[Feature Proposal] support setting custom metadata location prefix from catalog level #4583

Closed

Merge branch 'master' into tbl_defaults

f9e1ee4

Fix build

ed1a3a6

rajarshisarkar added 2 commits May 11, 2022 17:08

Resolve review comments

6af5b51

Resolve review comments

a2fbf59

singhpk234 reviewed May 12, 2022

View reviewed changes

Resolve review comments

9fe9215

kbendick reviewed May 17, 2022

View reviewed changes

rdblue reviewed May 17, 2022

View reviewed changes

rdblue approved these changes May 17, 2022

View reviewed changes

rdblue merged commit 06fcdbb into apache:master May 17, 2022

rajarshisarkar mentioned this pull request May 18, 2022

Catalog: Use ImmutableMap for catalog properties #4803

Merged

szehon-ho mentioned this pull request Jul 11, 2022

Core: Update MetricsConfig to use a default for first 32 columns #5215

Merged

szehon-ho mentioned this pull request Jul 22, 2022

Allow table read/write options to be configured and/or enforced at catalog level using catalog properties #5343

Closed

InvisibleProgrammer mentioned this pull request Dec 16, 2022

HIVE-26822: port changes before spotless apache/hive#3857

Closed

dramaticlly mentioned this pull request Apr 25, 2023

Update documentation to reflect new catalog features #7433

Merged

sungwy mentioned this pull request Feb 3, 2024

Catalog table-default and table-override properties apache/iceberg-python#362

Closed

szehon-ho mentioned this pull request Jul 10, 2024

Spark: Session level Iceberg table config defaults #2270

Closed

RussellSpitzer mentioned this pull request Jul 30, 2024

View Default Properties in the Catalog #10822

Closed

szehon-ho mentioned this pull request Nov 18, 2024

Spark 3.4: IcebergSource extends SessionConfigSupport #7732

Merged

		public static final String TABLE_DEFAULT_PREFIX = "table-default.";
		public static final String TABLE_OVERRIDE_PREFIX = "table-override.";

Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. #4011

Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. #4011

Uh oh!

Conversation

SinghAsDev commented Jan 30, 2022

Uh oh!

SinghAsDev commented Jan 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdblue commented Jan 30, 2022

Uh oh!

SinghAsDev commented Jan 31, 2022

Uh oh!

rdblue commented Jan 31, 2022

Uh oh!

SinghAsDev commented Feb 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajarshisarkar commented May 9, 2022

Uh oh!

SinghAsDev commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshisarkar commented May 10, 2022

Uh oh!

rajarshisarkar commented May 11, 2022

Uh oh!

rajarshisarkar commented May 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajarshisarkar May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rajarshisarkar commented May 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbendick left a comment

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

SinghAsDev commented May 9, 2022 •

edited

Loading

rajarshisarkar May 16, 2022 •

edited

Loading