Skip to content
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

Proposed SPI change for ConnectorMetadata.createView() #24442

Open
rschlussel opened this issue Jan 27, 2025 · 1 comment
Open

Proposed SPI change for ConnectorMetadata.createView() #24442

rschlussel opened this issue Jan 27, 2025 · 1 comment

Comments

@rschlussel
Copy link
Contributor

Motivation
Currently, all view metadata is encoded via json to a string before being sent to connectors (and similar process in reverse when they are retrieved). This makes it impossible for other systems to easily be able to understand presto views. For example, our security service would like to be able to understand the security mode of a view. decoding the json string is fragile, as the string is not actually guaranteed to have any particular kind of structure.

Proposal

I propose a change to the ConnectorMetadata SPI for create view.

Currently we have:

 default void createView(ConnectorSession session, ConnectorTableMetadata viewMetadata, String viewData, boolean replace)

Proposed new behavior - replace the viewData string with ViewDefinition

 default void createView(ConnectorSession session, ConnectorTableMetadata viewMetadata, ViewDefinition viewDefinition, boolean replace)

And then update Metadata/MetadataManager appropriately.

and similarly, getViews() would have option of returning the actual ViewDefinition.

Because we don't have a JsonCodec dependency in the SPI, we can't simply create a default implementation that encodes the viewDefinition and calls the old createView.

Options for handling the change:

  1. just change the SPI, and all connectors will need to adapt (can serialize to json themselves, similar to how hive also currently then base64 encodes the json string)
  2. leave 2 definitions, and have MetadataManager handle some try/catch of NOT_SUPPORTED exceptions for the new definition, then it calls the old one.
@tdcmeehan
Copy link
Contributor

This is a reasonable change. I think we should offer a brief deprecation period and essentially do both options:

  1. Do option 2, that is, leave both definitions in place. Deprecate the version that uses the Sringified viewData and announce this in the release notes that it's deprecated.
  2. Remove the old definition within the next 1-2 releases, and open a ticket for this task so we don't lose track.

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

No branches or pull requests

2 participants