[#1661] feat(core): introduce catalog capability framework #2819
Conversation
mchades
force-pushed
the
issue-1661
branch
2 times, most recently
from
0bc6e19 to
695e959
Compare
|
Could you please help review it, thanks! @jerryshao @diqiu50 |
| * some special capabilities, it should override the default implementation of the capabilities. | ||
| */ | ||
| @Evolving | ||
| public interface HasCapabilities { |
There was a problem hiding this comment.
I think the name HasCapabilities is not so proper here. I would call it Capability.
If you want to define a HasCapabilities interface? It should be like:
interface HasCapabilities {
Capability[] hasCapabilies()
}
core/src/main/java/com/datastrato/gravitino/catalog/CapabilitiesHelpers.java
Outdated
Show resolved
Hide resolved
| * @return A new instance of HasCapabilities. | ||
| */ | ||
| @Evolving | ||
| protected HasCapabilities newCaps() { |
There was a problem hiding this comment.
I don't suggest use the abbreviation "caps" here, it is not a common abbreviation.
| TABLE, | ||
| COLUMN, | ||
| FILESET | ||
| } |
There was a problem hiding this comment.
Should you also add messaging and partition?
|
|
||
| public class CapabilityHelpers { | ||
|
|
||
| public static Column[] applyCapabilities(Column[] columns, Capability capabilities) { |
There was a problem hiding this comment.
It is just one capability, why do you need to use plural?
There was a problem hiding this comment.
This method will apply multiple capabilities to the column, such as default value, not null, case sensitivity, and so on. Therefore I use plural
| Column appliedColumn = | ||
| applyCapabilities( | ||
| Column.of( | ||
| addColumn.fieldName()[0], |
There was a problem hiding this comment.
Why getting the first element of the fieldName?
There was a problem hiding this comment.
because the first name is the column name, and the others are just nested fields in columns.
What changes were proposed in this pull request?
column not nullcapability to show how the framework worksWhy are the changes needed?
Improving code quality
Fix: #1662
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests