-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38848] [table] Supports to pass the required privilege when catalog manager gets the table #27389
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
base: master
Are you sure you want to change the base?
Conversation
036e08f to
f85b2e8
Compare
| resolveCatalogBaseTable(temporaryTable); | ||
| return Optional.of(ContextResolvedTable.temporary(objectIdentifier, resolvedTable)); | ||
| } else { | ||
| return getPermanentTable(objectIdentifier, 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.
I think this is an exciting addition, I think it is big enough to warrant a Flip.
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.
This pull request includes all the required code. It doesn't have many changes. FLIP may need many code changes in my opinion.
If you think that we should start a discussion thread in the dev mail list, I can start the discussion in the dev mail list.
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 Thankyou I suggest starting a discussion on the dev list to see if it warrants a Flip.
…nager loads the table
|
@flinkbot run azure |
|
@twalthr Could u help me review this pull request? |
| tableEnvironment.getParser().parseIdentifier(tablePath); | ||
| ObjectIdentifier objectIdentifier = | ||
| tableEnvironment.getCatalogManager().qualifyIdentifier(unresolvedIdentifier); | ||
| Set<TableWritePrivilege> privileges = Sets.newHashSet(TableWritePrivilege.INSERT); |
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.
this needs to have junit tests added to drive the new code paths
What is the purpose of the change
For custom catalogs that have access control, read and write permissions can be different. However, currently Flink always call Catalog#getTable to look up the table, no matter it's for read or write.
This PR adds a variant of getTable that indicates the required write privileges. All the write commands will call this new method to look up tables instead. This new method has a default implementation that just calls getTable, so there is no breaking change.
Apache Spark also has similar interfaces. It helps catalog to implementation access control feature better.
More details, you can see apache/spark#47772
Brief change log
I added interfaces getTable with required privileges for
CatalogManagerandCatalog.I added TableWritePrivileges, include insert, update, delete.
I passed the write privileges in the
TableImpl.javaandSqlNodeToOperationConversion.java.Verifying this change
Run the existing tests to verify that this change won't break compatibility.
I also run the
flink-sql-clientto verify the sqlinsert,update,deleteusing a custom catalog service (Apache Gravitino). It takes effect.Does this pull request potentially affect one of the following parts:
@Public(Evolving): Yes, I added the enumTableWritePrivilegesand modified someCataloginterfaces. They are compatible changes.Documentation