Skip to content

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Sep 3, 2025

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Sep 3, 2025
@dolfinus
Copy link
Contributor

dolfinus commented Sep 3, 2025

For JDBC-based connectors this is fine, but there are other connectors with different connection url format. Hive uses thrift://..., Kafka uses list of host:port of brokers, Loki uses http://. In most cases these can be represented in string form, but formats are different for each connector. Also, Google Sheets don't have connection URL at all, so nothing to parse.

To handle these connection URLs differently, there should be at least TableInfo.connectorName field. TableInfo.catalog field contains catalog name, not connector name.

@wendigo
Copy link
Contributor Author

wendigo commented Sep 3, 2025

We can add a connector name to TableInfo. The location should be connector agnostic while being part of TableInfo (SPI implementations shouldn't leak there) so Optional<String> would need to do

@dolfinus
Copy link
Contributor

dolfinus commented Sep 3, 2025

Sounds reasonable.

One more thing though - TableInfo.location can make users think that this represents something like CREATE TABLE iceberg.mytable (...) LOCATION 's3://bucket/warehouse/mytable'. So I suggest using connectionURL/connectorAddress naming instead.

@wendigo
Copy link
Contributor Author

wendigo commented Sep 3, 2025

@dolfinus that wouldn't make sense in the context of the lake connectors where you want s3://bucket/table/location

@dolfinus
Copy link
Contributor

dolfinus commented Sep 3, 2025

I mean that these should be separated options. Table location for lake connectors is one of table properties called location, and Catalog connector URL is a different thing which should not be named TableInfo.location.

@chenjian2664
Copy link
Contributor

@dolfinus I was expecting the location to be within the table’s domain. I don't understand why we need to involve the catalog perspective

@dolfinus
Copy link
Contributor

dolfinus commented Sep 5, 2025

@chenjian2664 Have you checked the issue #25555?

@@ -38,6 +39,7 @@

public final class JdbcTableHandle
extends BaseJdbcConnectorTableHandle
implements ConnectorTableLocation
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have a SPI specific API in ConnectorMetadata which would resolve location based on ConnectorTableHandle ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed ignite Ignite connector
Development

Successfully merging this pull request may close these issues.

4 participants