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

[#317] feat(core, catalog): make fields in AuditInfo optional and overwriteable #330

Merged
merged 4 commits into from
Sep 6, 2023

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

This PR proposes to change the requirements of the AuditInfo fields to make them optional and overwriteable.

Why are the changes needed?

This is the first change of #250, the change is going to address two problems:

  1. If the AuditInfo is not existed in both Graviton store and underlying source, we should support the empty AuditInfo, or only several fields are set in AuditInfo.
  2. If the AuditInfo are both set in the Graviton store and underlying source, we should support AuditInfo mergeable.

Fix: #317

Does this PR introduce any user-facing change?

No

How was this patch tested?

Modify and add the UTs to test.

@jerryshao jerryshao self-assigned this Sep 6, 2023
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Code Coverage Report

Overall Project 62.89% -0.19% 🟢
Files changed 79.55% 🟢

Module Coverage
core 71.04% -0.58% 🟢
catalog-hive 59.11% 🟢
common 39.88% 🟢
Files
Module File Coverage
core AuditInfoSerDe.java 100% 🟢
AuditInfo.java 62.5% -15.34% 🔴
catalog-hive HiveTable.java 94.51% 🟢
HiveSchema.java 84.21% 🟢
HiveCatalogOperations.java 66.42% 🟢
common MetalakeListResponse.java 65.18% 🟢
MetalakeResponse.java 63.11% 🟢
CatalogResponse.java 61.83% 🟢
TableResponse.java 37.5% 🟢
SchemaResponse.java 31.15% 🟢

.withLastModifier(currentUser())
.withLastModifiedTime(Instant.now())
.build();
alteredTable.auditInfo().merge(newAudit, true /* overwrite */);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Does the code mean that the audit information in gravition is more recent than that in external storage, and that's what we'll use as the final result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the audit info also exists in Graviton, we will use the one from Graviton as precendence.

mchades
mchades previously approved these changes Sep 6, 2023
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM

@jerryshao jerryshao merged commit d9dfd48 into apache:main Sep 6, 2023
2 checks passed
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.

[Subtask] Refactor the current AuditInfo related code and push the implementation to each catalog
3 participants