Skip to content

Conversation

@jerqi
Copy link

@jerqi jerqi commented Jan 7, 2026

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 CatalogManager and Catalog.

I added TableWritePrivileges, include insert, update, delete.

I passed the write privileges in the TableImpl.java and SqlNodeToOperationConversion.java.

Verifying this change

Run the existing tests to verify that this change won't break compatibility.
I also run the flink-sql-client to verify the sql insert, update, delete using a custom catalog service (Apache Gravitino). It takes effect.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): Yes, I added the enum TableWritePrivileges and modified some Catalog interfaces. They are compatible changes.
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@jerqi jerqi marked this pull request as draft January 7, 2026 03:43
@jerqi jerqi changed the title [FLINK-38848] Supports to pass the required privilege when catalog manager loads the table [FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table Jan 7, 2026
@jerqi jerqi changed the title [FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table [#FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table Jan 7, 2026
@jerqi jerqi changed the title [#FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table [FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table Jan 7, 2026
@flinkbot
Copy link
Collaborator

flinkbot commented Jan 7, 2026

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@jerqi jerqi force-pushed the FLINK-38848 branch 3 times, most recently from 036e08f to f85b2e8 Compare January 7, 2026 06:35
@jerqi jerqi closed this Jan 7, 2026
@jerqi jerqi reopened this Jan 7, 2026
@jerqi jerqi changed the title [FLINK-38848] [table] Supports to pass the required privilege when catalog manager loads the table [FLINK-38848] [table] Supports to pass the required privilege when catalog manager gets the table Jan 7, 2026
@jerqi jerqi closed this Jan 7, 2026
@jerqi jerqi reopened this Jan 7, 2026
resolveCatalogBaseTable(temporaryTable);
return Optional.of(ContextResolvedTable.temporary(objectIdentifier, resolvedTable));
} else {
return getPermanentTable(objectIdentifier, null);
Copy link
Contributor

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.

Copy link
Author

@jerqi jerqi Jan 8, 2026

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.

Copy link
Contributor

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.

@github-actions github-actions bot added the community-reviewed PR has been reviewed by the community. label Jan 8, 2026
@jerqi
Copy link
Author

jerqi commented Jan 8, 2026

@flinkbot run azure

@jerqi jerqi marked this pull request as ready for review January 8, 2026 06:07
@jerqi
Copy link
Author

jerqi commented Jan 8, 2026

@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);
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-reviewed PR has been reviewed by the community.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants