Skip to content
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

[#4860] improvement(core): Add a skeleton for EntityStore #4861

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Sep 5, 2024

What changes were proposed in this pull request?

Propose a new interface for EntityStore based on https://docs.google.com/document/d/11I-qyXrarQhCMZXoI5d1z5OEt8UlrpHm6Ix07DOuL4Y/edit#heading=h.vs40sx9ghyu2

Why are the changes needed?

The current EntityStore is not elegant enough.

Fix: #4840

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

N/A.

@yuqi1129 yuqi1129 self-assigned this Sep 5, 2024
@yuqi1129 yuqi1129 marked this pull request as draft September 5, 2024 07:52
* @param type RelationType of the relationship.
* @throws AlreadyExistsException if the relation already exists.
*/
void addRelation(
Copy link
Contributor

@jerqi jerqi Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support to override relation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I analyzed the current call chain and it doesn't seem necessary to add an update interface. In order to make it more extensive, I agree at it's more preferable to add an update funtionity.

* @return List of MetadataObject that are related to the starting point.
* @throws IOException if the list operation fails.
*/
List<MetadataObject> listMetadataObjectsByRelation(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between listEntitiesByRelation and listMetadataObjectsByRelation?

Copy link
Contributor Author

@yuqi1129 yuqi1129 Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are utilized in different scenarios.

  • listEntitiesByRelation is used to list all entities by a relation, all these entities should be with the same type, for example, there can be all metalake or catalogs, currently we can't get a bundle of entities with different entity types.
  • listMetadataObjectsByRelation handles the case that listEntitiesByRelation can't cope with and only returns entities identifier.

Do you have any good suggestions for merging these two, I'm also trying to combine them, the current solution is not so elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerqi
Currently the concepts of Entity and MetadataObject are quite different, and I can hardly find a good way to merge them.

Copy link
Contributor

@jerqi jerqi Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, we don't need to have the concept of metadata object in the EntityStore. The metadata object is a upper level concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we directly load the entity and then convert it to MetadataObject? If so, it's very heavy when loading/listing metadataobjects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok for me. The convert is the memory operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let me review it again.

@yuqi1129 yuqi1129 marked this pull request as ready for review September 9, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants