-
Notifications
You must be signed in to change notification settings - Fork 383
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
[#6133] Improvement(core): Supports get Fileset schema location in the AuthorizationUtils #6211
base: main
Are you sure you want to change the base?
Conversation
… in the AuthorizationUtils Supports get Fileset schema location in the AuthorizationUtils.
Hi @xunliu , could you please review this PR when you have time? I’d really appreciate your feedback. |
… in the AuthorizationUtils use StringUtils replace ==null.
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.
@Abyss-lord
I think we needs to consider to add an integration test, It's include Schema is Fileset.
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/hadoop/Constants.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
Show resolved
Hide resolved
break; | ||
|
||
case FILESET: | ||
if (catalogObj instanceof HasPropertyMetadata) { |
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.
Why do we need to judge HasPropertyMetdatadata
?
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.
@jerqi Is the fileset catalog guaranteed to implement the HasPropertyMetadata
interface?
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.
Some of the fileset catalogs have the property location
although they implement the HasPropertyMetdata
interface.
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.
The current processing logic is
- If the provider is Hadoop, use
HasPropertyMetadata
to get the location of the Schema. - In other cases, consider adding paths to all filesets below the Schema.
WDYT, @jerqi
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.
The 2 seems weird for me. Is this consistent with Hive schema implement.
What changes were proposed in this pull request?
Supports get Fileset schema location in the AuthorizationUtils
A Fileset can be uniquely identified using
metalake.catalog.schema.fileset
. The logic for retrieving the schema is as follows:Check the type of
catalogObj.type()
:RELATIONAL
, determine whether theprovider
is Hive. If it is a Hive table, retrieve itsLOCATION
property.FILESET
, determine whether it implementsHasPropertyMetadata
:schemaPropertiesMetadata()
method to retrieve the path.HasPropertyMetadata
, check whether it contains any Fileset objects:FilesetCatalog
and retrieve itsLOCATION
property.Why are the changes needed?
Fix: #6133
Does this PR introduce any user-facing change?
No
How was this patch tested?
local test.