[#91]feat(catalog): Hive schema entity serde and store support #208

mchades · 2023-08-10T06:29:20Z

What changes were proposed in this pull request?

Add CommonSchema class and Schema proto for serde
serde and store HiveSchema as CommonSchema
hive schema operations support Graviton store

Why are the changes needed?

we could store the additional entity information to our own storage.

Fix: #91

Does this PR introduce any user-facing change?

Add some check for Graviton store

How was this patch tested?

UTs added

github-actions · 2023-08-10T06:31:08Z

Code Coverage Report

Overall Project	59.58% `-0.9%`	🟢
Files changed	53.11%	🔴

Module	Coverage
core	67.23% `-1.04%`	🔴
catalog-hive	59.5% `-3.73%`	🟢

Files

Module	File	Coverage
core	ProtoEntitySerDe.java	63.14%	🟢
	BaseSchema.java	58.13% `-6.88%`	🔴
	GravitonEnv.java	0% `-4.59%`	🔴
	SchemaEntitySerDe.java	0%	🔴
catalog-hive	HiveCatalogOperations.java	69.56% `-8.21%`	🟢

jerryshao · 2023-08-10T07:49:08Z

@yuqi1129 , please help to review this.

Done

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

mchades · 2023-08-10T12:39:04Z

OK, Is it possible to move hive-related operations out of the backend transaction method. I still think it's a litter weird

@yuqi1129 If we move hive-related operations out of the backend transaction method, once hive-related failed, the Graviton store will not rollback

core/src/main/java/com/datastrato/graviton/proto/AuditInfoSerDe.java

jerryshao · 2023-08-11T06:40:14Z

core/src/main/java/com/datastrato/graviton/meta/rel/BaseSchema.java

+
+      return commonSchema;
+    }
+  }


I personally don't like this class CommonSchema here. CommonSchema has nothing to do with HiveSchema. Semantically you'll both have CommonSchema and HiveSchema, but actually CommonSchema is just a placeholder for schema info, not an entity.

If it is a placeholder, then it should have a different name, if it is a base class of HiveSchema, then the class organization should be changed.

My original intention was to use commonSchema as a generic class for storing basic information. Any entity that does not require additional information can be serialized and deserialized using commonSchema.

Another mainly reason for needing the commonSchema class is that BaseSchema is an abstract class and cannot be instantiated.

If so, I think we can have several options:

Remove abstract limitation for BaseSchema class.

Directly create HiveSchema's serde to proto Schema.

Basically, each entity (with identifier) should be mapping to a metadata object. Semantically CommonSchema and HiveSchema are two different entities if they don't have inheritance.

got it, option one was chosen

jerryshao · 2023-08-11T06:43:59Z

meta/src/main/proto/graviton_meta.proto

+  uint64 catalog_id = 2;
+  string name = 3;
+  optional string comment = 4;
+  map<string, string> properties = 5;


I don't think we need to store comment/properties, they will be stored in hive?

jerryshao · 2023-08-11T06:45:26Z

core/src/main/java/com/datastrato/graviton/proto/SchemaEntitySerDe.java

+        .setName(schemaEntity.name())
+        .setAuditInfo(AuditInfoSerDe.ser(schemaEntity.auditInfo()))
+        .setComment(schemaEntity.getComment())
+        .putAllProperties(schemaEntity.getProperties())


Why don't you check the nullability of comment and properties?

comment and properties are removed for now.

jerryshao · 2023-08-11T06:48:14Z

core/src/main/java/com/datastrato/graviton/proto/ProtoEntitySerDe.java

+          "com.datastrato.graviton.meta.rel.BaseSchema.CommonSchema",
+          "com.datastrato.graviton.proto.SchemaEntitySerDe",
+          "com.datastrato.graviton.catalog.hive.HiveSchema",
+          "com.datastrato.graviton.proto.SchemaEntitySerDe");


why do you both have commonSchema and HiveSchema here?

If you put CommonSchema and SchemaEntitySerDe here in core module, why do you meet class not found issue?

Let's review the serde procedure for HiveSchema as follows:

serialize:

Find SchemaEntitySerDe class according to HiveSchema class.

SchemaEntitySerDe serialize HiveSchema entity to Schema proto

deserialize:

Find Schema proto class according to passed parameter class of HiveSchema

deserialize data from bytesArray to Schema proto instance

Find SchemaEntitySerDe class according to passed parameter class of HiveSchema

SchemaEntitySerDe deserialize from Schema proto to BaseSchema instance

With above describtioin, we really don't need map key of CommonSchema(BaseSchema), I have removed CommonSchema.

jerryshao · 2023-08-11T06:49:34Z

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

-            client.dropDatabase(ident.name(), false, false, cascade);
+      store.executeInTransaction(
+          () -> {
+            store.delete(ident, SCHEMA);


do you need to delete all tables under this schema?

yeah, delete tables operation added.

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

jerryshao · 2023-08-14T02:43:11Z

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

+    } catch (EntityAlreadyExistsException e) {
+      throw new SchemaAlreadyExistsException(
+          String.format(
+              "Hive schema (database) '%s' already exists in Graviton store", ident.name()));


A blank line after each exception to make the code more clear.

jerryshao · 2023-08-14T02:52:48Z

core/src/main/java/com/datastrato/graviton/proto/ProtoEntitySerDe.java

@@ -24,13 +24,20 @@ public class ProtoEntitySerDe implements EntitySerDe {
          "com.datastrato.graviton.meta.BaseMetalake",
          "com.datastrato.graviton.proto.BaseMetalakeSerDe",
          "com.datastrato.graviton.meta.CatalogEntity",
-          "com.datastrato.graviton.proto.CatalogEntitySerDe");
+          "com.datastrato.graviton.proto.CatalogEntitySerDe",
+          "com.datastrato.graviton.catalog.hive.HiveSchema",


Here you're using HiveSchema with SchemaEntitySerDe, but the type parameter of SchemaEntitySerDe is BaseSchema, do we need to unify to BaseSchema?

This mapping indicates that the HiveSchema can utilize SchemaEntitySerDe.
In the future, we may also require other catalogs like xxxSchema (if xxx also do not require storing additional information) to make use of this schemaEntitySerDe, at that time, we can directly add a mapping relationship: xxxSchema -> SchemaEntitySerDe.

So the BaseSchema type parameter of SchemaEntitySerDe is used to reusing serder logic.

jerryshao · 2023-08-14T02:54:12Z

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

-      // TODO. We should also fetch the customized HiveSchema entity fields from our own
-      //  underlying storage, like id, auditInfo, etc.
+      EntityStore store = GravitonEnv.getInstance().entityStore();
+      BaseSchema baseSchema = store.get(ident, SCHEMA, HiveSchema.class);


Why do we use HiveSchema.class as a parameter, but the returned type is BaseSchema?

We need to the specific type of schema when converting schema from a byte array, what's is why we need HiveSchema.class not BaseSchema.class

In order to avoid confusion, I changed it to BaseSchema baseSchema = store.get(ident, SCHEMA, BaseSchema.class) here and then added BaseSchema serde mapping.

mchades requested a review from jerryshao August 10, 2023 06:29

mchades force-pushed the issue-91 branch from 7501630 to be8d473 Compare August 10, 2023 07:09

jerryshao requested a review from yuqi1129 August 10, 2023 07:48

yuqi1129 reviewed Aug 10, 2023

View reviewed changes

catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java Show resolved Hide resolved

catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java Show resolved Hide resolved

yuqi1129 reviewed Aug 10, 2023

View reviewed changes

yuqi1129 previously approved these changes Aug 10, 2023

View reviewed changes

jerryshao reviewed Aug 11, 2023

View reviewed changes

mchades dismissed yuqi1129’s stale review via 2438f28 August 11, 2023 08:26

mchades added 3 commits August 12, 2023 08:05

Hive schema entity serde and store support

a4f097a

remove comment/properties of schema from store

05b3690

remove abstract limitation of BaseSchema class

e30930e

mchades force-pushed the issue-91 branch from 2438f28 to e30930e Compare August 12, 2023 00:07

remove tables when drop schema

8855a95

jerryshao reviewed Aug 14, 2023

View reviewed changes

mchades added 2 commits August 14, 2023 11:03

format exception code

e8b95e1

add BaseSchema mapping

4b2de33

jerryshao approved these changes Aug 14, 2023

View reviewed changes

jerryshao merged commit bf288fa into apache:main Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#91]feat(catalog): Hive schema entity serde and store support #208

[#91]feat(catalog): Hive schema entity serde and store support #208

mchades commented Aug 10, 2023 •

edited by xunliu

Loading

github-actions bot commented Aug 10, 2023 •

edited

Loading

jerryshao commented Aug 10, 2023 •

edited by yuqi1129

Loading

mchades commented Aug 10, 2023

jerryshao Aug 11, 2023

mchades Aug 11, 2023

jerryshao Aug 11, 2023

mchades Aug 12, 2023

jerryshao Aug 11, 2023

mchades Aug 11, 2023

jerryshao Aug 11, 2023

mchades Aug 11, 2023

jerryshao Aug 11, 2023

mchades Aug 12, 2023

jerryshao Aug 11, 2023

mchades Aug 12, 2023

jerryshao Aug 14, 2023

mchades Aug 14, 2023

jerryshao Aug 14, 2023

mchades Aug 14, 2023

jerryshao Aug 14, 2023

yuqi1129 Aug 14, 2023

mchades Aug 14, 2023

[#91]feat(catalog): Hive schema entity serde and store support #208

[#91]feat(catalog): Hive schema entity serde and store support #208

Conversation

mchades commented Aug 10, 2023 • edited by xunliu 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 Aug 10, 2023 • edited Loading

Code Coverage Report

jerryshao commented Aug 10, 2023 • edited by yuqi1129 Loading

mchades commented Aug 10, 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

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 Aug 10, 2023 •

edited by xunliu

Loading

github-actions bot commented Aug 10, 2023 •

edited

Loading

jerryshao commented Aug 10, 2023 •

edited by yuqi1129

Loading