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

Refactor ConnectorMetadata to deprecate multi-layouts returning method #21767

Closed
wants to merge 0 commits into from

Conversation

hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Jan 24, 2024

Description

The support for multiple layouts has been removed in PR #12674, however the SPI method ConnectorMetadata.getTableLayouts() still return a List<ConnectorTableLayoutResult> which should always contains exactly one element. That's somehow unreasonable, and may lead in some confusion and inconvenience in the reading and subsequent development of the code. This PR try to deprecate this method, and replace it by getTableLayoutForConstraint() which explicitly return a ConnectorTableLayoutResult.

Test Plan

  • Make sure this refactor do not effect all existing test cases

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Deprecate SPI method ConnectorMetadata.getTableLayouts(), replace by ConnectorMetadata.getTableLayoutForConstraint()

@hantangwangd hantangwangd changed the title [For Test]Refactor ConnectorMetadata to avoid unnecessary multi-layouts returning Refactor ConnectorMetadata to avoid unnecessary multi-layouts returning Jan 24, 2024
@tdcmeehan tdcmeehan self-assigned this Jan 24, 2024
@tdcmeehan
Copy link
Contributor

I think we need to come up with a deprecation strategy.

For now, I would recommend adding a new method getTableLayoutForConstraint (this name makes it clear what it's supposed to do vs. getTableLayout which does not do filter pushdown). getTableLayouts can be marked as @Deprecated, and we can make its default implementation return a singleton list of getTableLayoutForConstraint.

In the release notes, we should mark that this method is being deprecated. Then, we should create an issue for X months down the road to delete the method.

getTableLayouts should always be identical to a singleton list of the result of getTableLayoutForConstraint. We can add a precondition check in MetadataManager#getLayout that proves this (probably where the current check to ensure size = 1 is). Then we can remove this code once deprecation is complete.

@hantangwangd
Copy link
Member Author

Thanks for the instruction. Sounds great, I will do it.

@hantangwangd hantangwangd changed the title Refactor ConnectorMetadata to avoid unnecessary multi-layouts returning Refactor ConnectorMetadata to deprecate multi-layouts returning method Feb 14, 2024
@hantangwangd
Copy link
Member Author

Hi @tdcmeehan, following your guidance, the new method getTableLayoutForConstraint has been added and getTableLayouts has been marked as @Deprecated, and the release note has been added. Consulting the PR #10278, perhaps for now it's better to make getTableLayoutForConstraint 's default implementation to validate and use getTableLayouts 's result. In this way, we can leave all connector implementations unchanged and let the engine's metadata manager to use the new SPI method. What's your opinion?

@@ -364,14 +364,10 @@ public TableLayoutResult getLayout(Session session, TableHandle table, Constrain
CatalogMetadata catalogMetadata = getCatalogMetadata(session, connectorId);
ConnectorMetadata metadata = catalogMetadata.getMetadataFor(connectorId);
ConnectorSession connectorSession = session.toConnectorSession(connectorId);
List<ConnectorTableLayoutResult> layouts = metadata.getTableLayouts(connectorSession, connectorTable, constraint, desiredColumns);
ConnectorTableLayoutResult layout = metadata.getTableLayoutForConstraint(connectorSession, connectorTable, constraint, desiredColumns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to ensure that the results between getTableLayouts and getTableLayoutForConstaint are consistent, otherwise it might be confusing to connector authors.

Remember that this method is also used by connectors not even known to Presto (i.e. external libraries). So we need to accomodate people who are slow to adopt to the new API.

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

Successfully merging this pull request may close these issues.

2 participants