-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#3607] feat(core): Support to import the entities when loading entities #3623
Changes from all commits
99b1caf
3dac56c
d5ca7e7
c831685
fe731f2
1b8f375
065d229
b5d5cd2
f39720a
08cf4e2
e762b58
ad5400a
e12c766
bde6b0d
870670e
8e2a7d3
e661e56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
|
||
import com.datastrato.gravitino.Audit; | ||
import com.datastrato.gravitino.Schema; | ||
import com.datastrato.gravitino.StringIdentifier; | ||
import com.datastrato.gravitino.meta.AuditInfo; | ||
import com.datastrato.gravitino.meta.SchemaEntity; | ||
import java.util.Map; | ||
|
@@ -19,14 +20,23 @@ | |
public final class EntityCombinedSchema implements Schema { | ||
|
||
private final Schema schema; | ||
|
||
private final SchemaEntity schemaEntity; | ||
|
||
// Sets of properties that should be hidden from the user. | ||
private Set<String> hiddenProperties; | ||
|
||
// Field "imported" is used to indicate whether the entity has been imported to Gravitino | ||
// managed storage backend. If "imported" is true, it means that storage backend have stored | ||
// the correct entity. Otherwise, we should import the external entity to the storage backend. | ||
// This is used for tag/access control related purposes, only the imported entities have the | ||
// unique id, and based on this id, we can label and control the access to the entities. | ||
private boolean imported; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should add some comments about what it was used for. What should we do if it's true or false? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Further information may be required, such as why we need to import it to the Gravitino-managed store. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@qqqttt123 do you take a look at these comments and check whether they need to be resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me update the doc. |
||
|
||
private EntityCombinedSchema(Schema schema, SchemaEntity schemaEntity) { | ||
this.schema = schema; | ||
this.schemaEntity = schemaEntity; | ||
this.imported = false; | ||
} | ||
|
||
public static EntityCombinedSchema of(Schema schema, SchemaEntity schemaEntity) { | ||
|
@@ -42,6 +52,11 @@ public EntityCombinedSchema withHiddenPropertiesSet(Set<String> hiddenProperties | |
return this; | ||
} | ||
|
||
public EntityCombinedSchema withImported(boolean imported) { | ||
this.imported = imported; | ||
return this; | ||
} | ||
|
||
@Override | ||
public String name() { | ||
return schema.name(); | ||
|
@@ -73,4 +88,12 @@ public Audit auditInfo() { | |
? schema.auditInfo() | ||
: mergedAudit.merge(schemaEntity.auditInfo(), true /* overwrite */); | ||
} | ||
|
||
public boolean imported() { | ||
return imported; | ||
} | ||
|
||
StringIdentifier stringIdentifier() { | ||
return StringIdentifier.fromProperties(schema.properties()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can handle this not in this PR, we may need a class named 'combinedEntity' to unify the handling of entities from different sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mchades what is the meaning here, can you explain more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean do we need to provide an abstract class
CombinedEntity<T>
forEntityCombinedSchema
,EntityCombinedTable
, andEntityCombinedTopic
?