Skip to content

Add support to provide an alias for a connector factory#8765

Closed
majetideepak wants to merge 1 commit intofacebookincubator:mainfrom
majetideepak:alias-hive-connector
Closed

Add support to provide an alias for a connector factory#8765
majetideepak wants to merge 1 commit intofacebookincubator:mainfrom
majetideepak:alias-hive-connector

Conversation

@majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Feb 16, 2024

The hive connector factory supports both hive-hadoop2 and iceberg connectors from Presto.
Add API to provide an alias to a ConnectorFactory.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 16, 2024
@netlify
Copy link

netlify bot commented Feb 16, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 12b5c58
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65dd5abf80ce5a00086a517c

@majetideepak majetideepak force-pushed the alias-hive-connector branch 2 times, most recently from a30aa7f to 98f9ba9 Compare February 16, 2024 10:25
@majetideepak majetideepak changed the title Add aliases in ConnectorFactory Add iceberg aliases in HiveConnectorFactory Feb 19, 2024
@majetideepak majetideepak changed the title Add iceberg aliases in HiveConnectorFactory Add iceberg alias in HiveConnectorFactory Feb 19, 2024
@majetideepak majetideepak changed the title Add iceberg alias in HiveConnectorFactory Add support to provide an alias for a connector factory Feb 19, 2024
@majetideepak majetideepak marked this pull request as ready for review February 19, 2024 11:07
@majetideepak
Copy link
Collaborator Author

@mbasmanova can you please take a look at this PR? Thanks!

/// Adds an alias for an existing factory. Throws if factory with the
/// connectorName is not already present. Throws if the alias for the
/// connectorName is already present.
bool registerConnectorFactoryAlias(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be better to change ConnectorFactory API to allow for aliases. Perhaps, add

const std::vector<std::string>& aliases() const;

method.

Or, modify registerConnectorFactory API to add an optional 'aliases' parameter. That would be similar to aliases allowed when registering scalar functions.

Copy link
Collaborator Author

@majetideepak majetideepak Feb 27, 2024

Choose a reason for hiding this comment

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

| I'm wondering if it would be better to change ConnectorFactory API to allow for aliases.

@mbasmanova This was my initial implementation. But we register the Velox connectors in the Velox library.
So an application such as Prestissimo would have to manually change the Velox code to include additional aliases. For example, to add "iceberg", we have to manually modify the alias list.
I then added this API so that we can add aliases in Prestissimo without having to change Velox code.

Copy link
Collaborator Author

@majetideepak majetideepak Feb 27, 2024

Choose a reason for hiding this comment

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

See the commit that is similar to your approach a30aa7f

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like Velox auto-registers connector factories and this is problematic. Let's change that to require applications to explicitly register what they need.

@majetideepak
Copy link
Collaborator Author

We should be able to register a new hive connector factory in the client.
See prestodb/presto#22710

@majetideepak majetideepak marked this pull request as draft May 9, 2024 20:39
@majetideepak
Copy link
Collaborator Author

Use prestodb/presto#22710 instead of this.

@majetideepak majetideepak deleted the alias-hive-connector branch October 5, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants