-
Notifications
You must be signed in to change notification settings - Fork 304
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
Conversation
Code Coverage Report
Files
|
catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveTable.java
Outdated
Show resolved
Hide resolved
.withLastModifier(currentUser()) | ||
.withLastModifiedTime(Instant.now()) | ||
.build(); | ||
alteredTable.auditInfo().merge(newAudit, true /* overwrite */); |
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'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?
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.
Yes, if the audit info also exists in Graviton, we will use the one from Graviton as precendence.
catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java
Show resolved
Hide resolved
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.
LGTM
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:
AuditInfo
is not existed in both Graviton store and underlying source, we should support the emptyAuditInfo
, or only several fields are set inAuditInfo
.AuditInfo
are both set in the Graviton store and underlying source, we should supportAuditInfo
mergeable.Fix: #317
Does this PR introduce any user-facing change?
No
How was this patch tested?
Modify and add the UTs to test.