[#412] feat(core): Generic property system support #441

mchades · 2023-09-22T15:51:43Z

What changes were proposed in this pull request?

Add the HasPropertyMetadata interface to BaseCatalog and CatalogOperations for acquiring PropertyMetadata
Add PropertyMetadata interface for tableProperty, schemaProperty and catalogProperty can also implement it later on
validate table properties when CatalogOperationDispatcher operates tables

Why are the changes needed?

Under the property framework, every catalog only needs to care about the table property metadata definition in itself, the work of property validation will be done by the framework.

Fix: #412

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT added in TestCatalogOperationDispatcher

github-actions · 2023-09-22T15:54:03Z

Code Coverage Report

Overall Project	62.92% `-0.2%`	🟢
Files changed	90.42%	🟢

Module	Coverage
core	77.02% `-0.57%`	🟢
catalog-lakehouse-iceberg	75.95% `-0.23%`	🟢
catalog-hive	60.7% `-0.12%`	🟢

Files

Module	File	Coverage
core	TablePropertiesMetadata.java	98.57% `-1.43%`	🟢
	PropertiesMetadata.java	97.1% `-2.9%`	🟢
	EntityCombinedTable.java	92.52%	🟢
	PropertyEntry.java	91.73% `-8.27%`	🟢
	CatalogOperationDispatcher.java	90.82% `-3.06%`	🟢
	CatalogManager.java	71.93%	🟢
	BaseCatalog.java	71.2% `-3.2%`	🔴
catalog-lakehouse-iceberg	IcebergCatalogOperations.java	71.73% `-0.63%`	🟢
	IcebergTablePropertiesMetadata.java	60% `-40%`	🟢
catalog-hive	HiveCatalogOperations.java	67.73% `-0.22%`	🟢
	HiveTablePropertiesMetadata.java	60% `-40%`	🟢

catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalog.java

core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java

core/src/main/java/com/datastrato/graviton/PropertySpec.java

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

FANNG1 · 2023-09-26T07:27:11Z

core/src/main/java/com/datastrato/graviton/PropertySpec.java

+      boolean required,
+      boolean immutable,
+      Class<T> javaType,
+      T defaultValue,


seems missing defaultValue process logic.

defaultValue is handled by each catalog separately.

why defaultValue is handled by each catalog? seems it's common

Because catalog maybe want to know has the user set this property, If we set defaultValue for all absent non-required properties in advance, the catalog can't tell if it was set by the user

Is there any property meets the scene you described? why should catalog differences it's set by user or graviton?

For instance, the location is a non-required property for a Hive table. If the user does not specify a location, Hive will automatically assign different values to different tables. However, if null is set as the default value for the location, it will result in a NPE in HMS.

location is special and shouldn't have a default value. HiveCatalog should assign a table location. For most cases, such as table format, which have a fixed default value, I think it's more suitable to be processed in common code.

HiveCatalog should assign a table location.

It's not necessary. For external table, location can also be assigned by user.

As you said, here does have some special properties that cannot be processed in common code, so I still think the defaultValue should be processed in catalog themselves

FANNG1 · 2023-09-26T08:05:58Z

core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java

+              () ->
+                  mergeTablePropertySpecs(
+                      basicCapability.tablePropertySpecs(),
+                      acquireCatalogCapability().tablePropertySpecs());


here, we mixed CatalogCapabity and TableCapabity, should we separate them? Take Iceberg for example,
catalogType is required when creating IcebergCatalog, while when creating IcebergTable, I don't want to specify catalogType again, it becomes reserved.

HiveCatalog. metastore.uri maybe have same logic

here, we mixed CatalogCapabity and TableCapabity

Here only process tablePropertySpecs, does not include CatalogPropertySpecs

FANNG1 · 2023-09-26T08:10:48Z

core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java

@@ -87,6 +87,10 @@ public <R> R doWithTableOps(ThrowableFunction<TableCatalog, R> fn) throws Except
          });
    }



when Create&Alter&Load catalog, should we check properties?

catalog property validation work in another pr #452

mchades · 2023-09-26T14:13:27Z

I have refactored the HasPropertyMetadata. Can you please help to review it again when you have time? thanks.
@jerryshao @yuqi1129

jerryshao · 2023-09-26T14:56:18Z

@mchades you need to update the PR description.

jerryshao · 2023-09-26T12:21:29Z

core/src/main/java/com/datastrato/graviton/PropertyEntry.java

+   * @param hidden Whether this property is hidden from user, such as password
+   * @param reserved This property is reserved and cannot be set by user
+   */
+  private PropertyEntry(


I would suggest to change to builder pattern.

jerryshao · 2023-09-26T12:22:02Z

core/src/main/java/com/datastrato/graviton/PropertyEntry.java

+  public static boolean isReservedProperty(
+      String propertyName, Map<String, PropertyEntry<?>> propertySpecs) {
+    return propertySpecs.containsKey(propertyName) && propertySpecs.get(propertyName).isReserved();
+  }
+
+  public static boolean isRequiredProperty(
+      String propertyName, Map<String, PropertyEntry<?>> propertySpecs) {
+    return propertySpecs.containsKey(propertyName) && propertySpecs.get(propertyName).isRequired();
+  }
+
+  public static boolean isImmutableProperty(
+      String propertyName, Map<String, PropertyEntry<?>> propertySpecs) {
+    return propertySpecs.containsKey(propertyName) && propertySpecs.get(propertyName).isImmutable();
+  }
+
+  public static boolean isHiddenProperty(
+      String propertyName, Map<String, PropertyEntry<?>> propertySpecs) {
+    return propertySpecs.containsKey(propertyName) && propertySpecs.get(propertyName).isHidden();
+  }


Are these better to move to PropertyMetadata?

jerryshao · 2023-09-26T12:22:44Z

core/src/main/java/com/datastrato/graviton/PropertyEntry.java

+  }
+
+  // need remove?
+  public static Map<String, String> encodeProperties(


If not used, then we can remove it.

jerryshao · 2023-09-26T15:30:37Z

catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalog.java

@@ -31,7 +32,7 @@ public String shortName() {
   */
  @Override
  protected CatalogOperations newOps(Map<String, String> config) {
-    HiveCatalogOperations ops = new HiveCatalogOperations(entity());
+    HiveCatalogOperations ops = new HiveCatalogOperations(entity(), tableProperty());


I think you don't have to create xxxProperty() here and pass in to the specific HiveCatalogOperations. Instead, you can let HiveCatalogOperations implements HasPropertyMetadata interface, and in catalog, we can change to:

@Override public PropertyMetadata tableProperty() throws UnsupportedOperationException { return ops.tableProperty(); }

OK, I have let CatalogOperations implements HasPropertyMetadata interface to make sure catalog developers implement the interface.

BTW, the PR description already updated.

fix Iceberg rename ability to capability fix typo add illegal combination validation in PropertySpec remove alterTable and loadTable wrapper remove generics V refact split HasPropertyMetadata interface PropertyEntry use builder pattern move static method to PropertyMetadata

core/src/main/java/com/datastrato/graviton/PropertyMetadata.java

...s/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java

jerryshao · 2023-09-27T03:17:07Z

core/src/main/java/com/datastrato/graviton/PropertyEntry.java

+    return decoder.apply(value);
+  }
+
+  public static PropertyEntry<String> stringProperty(


Is it better changing to stringPropertyEntry?

Also I think you can add several help methods like stringRequiredProperty, stringReservedProperty to simplify the use.

I have added stringRequiredProperty, stringReservedProperty and stringImmutablePropertyEntry method for current usage and test, and will add more help methods in catalog development later on, since there are many flag combinations.

jerryshao · 2023-09-27T03:25:32Z

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

+    return table;
+  }
+
+  private Table loadTableWithAllProperties(NameIdentifier ident) {


Is it necessary to split to a new method, I think changing the loadTable should be enough?

Since the original loadTable method will verify the ID_KEY property, but the property framework will remove hidden property after loadTable, so I renamed the original loadTable to loadTableWithAllProperties for reuse the logic.

core/src/main/java/com/datastrato/graviton/PropertyEntry.java

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java

core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java

… catalog package

core/src/main/java/com/datastrato/graviton/PropertyMetadata.java

yuqi1129 · 2023-09-27T07:58:17Z

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

+    for (TableChange change : changes) {
+      if (change instanceof TableChange.SetProperty) {
+        String propertyName = ((TableChange.SetProperty) change).getProperty();
+        if (isReservedProperty(propertyName, tablePropertySpecs)) {


I'm not very clear here: Why here is not the same as line 514, as both SetProperty and RemoveProperty will change the property value. If the value of property is unchanging, it should fail here.

ImmutableProperty means the property cannot be changed by user, but if the property has not been set before, user can set it.

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

core/src/main/java/com/datastrato/graviton/TableProperty.java

core/src/main/java/com/datastrato/graviton/catalog/PropertyEntry.java

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

core/src/main/java/com/datastrato/graviton/catalog/rel/EntityCombinedTable.java

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java

### What changes were proposed in this pull request? 1. Add the `HasPropertyMetadata` interface to `BaseCatalog` and `CatalogOperations` for acquiring `PropertyMetadata` 2. Add `PropertyMetadata` interface for `tableProperty`, `schemaProperty` and `catalogProperty` can also implement it later on 3. validate table properties when `CatalogOperationDispatcher` operates tables ### Why are the changes needed? Under the property framework, every catalog only needs to care about the table property metadata definition in itself, the work of property validation will be done by the framework. Fix: #412 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT added in `TestCatalogOperationDispatcher` --------- Co-authored-by: yuqi <yuqi@datastrato.com>

mchades marked this pull request as ready for review September 25, 2023 13:05

mchades force-pushed the property branch from 74b2715 to 38b387f Compare September 25, 2023 13:10

mchades requested review from jerryshao, yuqi1129 and FANNG1 September 25, 2023 13:36

mchades self-assigned this Sep 26, 2023

jerryshao reviewed Sep 26, 2023

View reviewed changes

FANNG1 reviewed Sep 26, 2023

View reviewed changes

jerryshao reviewed Sep 26, 2023

View reviewed changes

mchades added 2 commits September 27, 2023 10:28

CatalogOperations interface extends HasPropertyMetadata

c2892ad

mchades force-pushed the property branch from 266091f to c2892ad Compare September 27, 2023 02:33

remove redundant inteface implements in HiveCatalogOperations

d6aa917

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/PropertyEntry.java Outdated Show resolved Hide resolved

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java Outdated Show resolved Hide resolved

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java Show resolved Hide resolved

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/HasPropertyMetadata.java Show resolved Hide resolved

mchades added 3 commits September 27, 2023 13:58

resolve name problem

dac41cd

add PropertyEntry help method

e0e9d37

move TablePropertiesMetadata, PropertyEntry and PropertiesMetadata to…

d3db783

… catalog package

yuqi1129 reviewed Sep 27, 2023

View reviewed changes

mchades added 3 commits September 27, 2023 16:59

resolve classLoad isolate

5cc7810

add javadoc

7a95db9

improve method

0e2c280

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/PropertyEntry.java Outdated Show resolved Hide resolved

mchades added 2 commits September 27, 2023 19:21

improve remove hidden properties

9734789

fix builder

a146bc2

mchades closed this Sep 27, 2023

mchades reopened this Sep 27, 2023

mchades closed this Sep 27, 2023

mchades reopened this Sep 27, 2023

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java Outdated Show resolved Hide resolved

jerryshao reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/rel/EntityCombinedTable.java Outdated Show resolved Hide resolved

yuqi1129 reviewed Sep 27, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java Outdated Show resolved Hide resolved

yuqi1129 added 2 commits September 27, 2023 20:39

Fix comments

084dfe1

Fix comments about hiddenProperties

c8951e7

jerryshao approved these changes Sep 27, 2023

View reviewed changes

jerryshao merged commit 33a7d2b into apache:main Sep 27, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#412] feat(core): Generic property system support #441

[#412] feat(core): Generic property system support #441

mchades commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading

FANNG1 Sep 26, 2023

mchades Sep 26, 2023

FANNG1 Sep 27, 2023

mchades Sep 27, 2023

FANNG1 Sep 27, 2023 •

edited

Loading

mchades Sep 27, 2023

FANNG1 Sep 27, 2023

mchades Sep 28, 2023

FANNG1 Sep 26, 2023

FANNG1 Sep 26, 2023

mchades Sep 26, 2023 •

edited

Loading

FANNG1 Sep 26, 2023

mchades Sep 26, 2023

mchades commented Sep 26, 2023

jerryshao commented Sep 26, 2023

jerryshao Sep 26, 2023

mchades Sep 27, 2023

jerryshao Sep 26, 2023

mchades Sep 27, 2023

jerryshao Sep 26, 2023

mchades Sep 27, 2023

jerryshao Sep 26, 2023

mchades Sep 27, 2023

jerryshao Sep 27, 2023

jerryshao Sep 27, 2023

mchades Sep 27, 2023

jerryshao Sep 27, 2023

mchades Sep 27, 2023

yuqi1129 Sep 27, 2023

mchades Sep 27, 2023

		@@ -87,6 +87,10 @@ public <R> R doWithTableOps(ThrowableFunction<TableCatalog, R> fn) throws Except
		});
		}

[#412] feat(core): Generic property system support #441

[#412] feat(core): Generic property system support #441

Conversation

mchades commented Sep 22, 2023 • edited Loading

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?

github-actions bot commented Sep 22, 2023 • edited Loading

Code Coverage Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades commented Sep 26, 2023

jerryshao commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading

FANNG1 Sep 27, 2023 •

edited

Loading

mchades Sep 26, 2023 •

edited

Loading