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

[#4305] improvement(core): Improved the way of fill parentEntityId in POBuilder #6114

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

luoshipeng
Copy link
Contributor

What changes were proposed in this pull request?

remove the foreach statement, and get parentEntityId directly.

Why are the changes needed?

issue: #4305

Does this PR introduce any user-facing change?

No

How was this patch tested?

ut

@jerqi
Copy link
Contributor

jerqi commented Jan 6, 2025

Could you extract a common method convert namespace to a list of entity id? Every method can reuse this method.

@luoshipeng
Copy link
Contributor Author

Could you extract a common method convert namespace to a list of entity id? Every method can reuse this method.

OK

@luoshipeng
Copy link
Contributor Author

Could you extract a common method convert namespace to a list of entity id? Every method can reuse this method.

Hi~ Could you take a look pls

Preconditions.checkArgument(
!namespace.isEmpty() && namespace.levels().length <= 3,
"Namespace should not be empty and length should be less than or equal to 3.");
Long[] parentEntityIds = new Long[3];
Copy link
Contributor

Choose a reason for hiding this comment

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

The length should be the same as namespace's.

@@ -57,4 +57,29 @@ public Long getParentEntityIdByNamespace(Namespace namespace) {
"Parent entity id should not be null and should be greater than 0.");
return parentEntityId;
}

public Long[] getParentEntityIdsByNamespace(Namespace namespace) {
Preconditions.checkArgument(
Copy link
Contributor

Choose a reason for hiding this comment

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

The length of schema won't be 3.

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 this is the same problem as the last one?

Copy link
Contributor

Choose a reason for hiding this comment

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

If namespace belongs to a schema, the length of namespace will be 2 instead of 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. so if i update the array length to namespace's length, it will solve this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to replace the similar code of ColumnMetaService?

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've checked the code of ColumnMetaService, and not found the similar code. Did i miss that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ColumnPO get parentEntityId from TablePO, so i think there's no need to replace it

Copy link
Contributor

Choose a reason for hiding this comment

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

ColumnPO get parentEntityId from TablePO, so i think there's no need to replace it

OK. I got it.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM.

@xunliu
Copy link
Member

xunliu commented Jan 7, 2025

hi @luoshipeng Thank you for your contributions.

@xunliu xunliu merged commit bec35a6 into apache:main Jan 7, 2025
28 checks passed
CommonMetaService.getInstance().getParentEntityIdsByNamespace(namespace);
builder.withMetalakeId(parentEntityIds[0]);
builder.withCatalogId(parentEntityIds[1]);
builder.withSchemaId(parentEntityIds[2]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can change to this style for here and below.

    builder.withMetalakeId(parentEntityIds[0])
          .withCatalogId(parentEntityIds[1]);
          .withSchemaId(parentEntityIds[2]);

Abyss-lord pushed a commit to Abyss-lord/gravitino that referenced this pull request Jan 9, 2025
…yId in POBuilder (apache#6114)

### What changes were proposed in this pull request?

remove the for each statement, and get parentEntityId directly.

### Why are the changes needed?

issue: apache#4305

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

ut

---------

Co-authored-by: luoshipeng <[email protected]>
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.

4 participants