feat(plugin-trino): add Trino driver over the REST client protocol#1910
feat(plugin-trino): add Trino driver over the REST client protocol#1910datlechin wants to merge 6 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a940f66d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| let initialRequest = makeRequest(method: .post, url: statementURL, headers: initialHeaders(), body: Data(sql.utf8)) | ||
| var httpResponse = try await sendWithRetry(initialRequest) |
There was a problem hiding this comment.
sendWithRetry retries 502/503/504/429, and using it for the initial POST /v1/statement can resubmit the same SQL after a gateway or proxy returns a transient error even though the coordinator already accepted the statement. For INSERT/DDL or connector side-effecting statements this can execute twice; keep retries for nextUri GETs or require an idempotency mechanism before retrying the POST.
Useful? React with 👍 / 👎.
| static let supportsDropIndex = false | ||
| static let supportsModifyPrimaryKey = false | ||
| static let structureColumnFields: [StructureColumnField] = [.name, .type, .nullable, .defaultValue] | ||
| static let postConnectActions: [PostConnectAction] = [.selectSchemaFromLastSession] |
There was a problem hiding this comment.
With Trino the catalog is the database dimension: switchDatabase(to:) stores it in TrinoSessionState, and the app persists that selection as the last database. On reconnect, DatabaseManager+Sessions.executePostConnectActions restores the saved database only when .selectDatabaseFromLastSession is present; this metadata restores only the schema, so connections whose catalog was selected in the UI come back with no catalog and schema/table browsing stays empty until the user reselects a catalog. Include the database action before the schema action, and mirror it in the registry default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e284fe66a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if upper.hasPrefix("TIME") { | ||
| return .timestamp(rawType: rawTypeName) |
There was a problem hiding this comment.
For Cassandra timeuuid columns, the classifier reaches this fallback with upper == "TIMEUUID" because that raw type is not in typeLookup; this new broad prefix check now classifies UUID strings as timestamps, giving them date/time formatting and filtering instead of textual/UUID behavior. Restrict this branch to actual Trino time spellings such as TIME WITH TIME ZONE rather than every type beginning with TIME.
Useful? React with 👍 / 👎.
| public func executeStreamed(_ sql: String) -> AsyncThrowingStream<TrinoStreamElement, Error> { | ||
| AsyncThrowingStream { continuation in | ||
| let client = self | ||
| Task { |
There was a problem hiding this comment.
When a streaming export or other caller stops consuming the returned AsyncThrowingStream, this unstructured producer task is not tied to continuation.onTermination, so it can keep polling nextUri and leave the Trino query running on the cluster after the UI/export was cancelled. Store the task and cancel it from the termination handler so runStatement observes cancellation and deletes the current URI.
Useful? React with 👍 / 👎.
| func columnTypeKey(schema: String?, table: String) -> String { | ||
| "\(resolveSchema(schema) ?? "").\(table)" |
There was a problem hiding this comment.
After switching catalogs, two tables with the same schema and table name share this cache entry even though Trino catalogs are independent. Since fetchColumns populates the cached types and generateStatements uses them to decide whether edited values should be quoted or cast, editing schema.table in a second catalog can reuse stale types from the first catalog; include currentCatalog in the key or clear this cache on catalog switch.
Useful? React with 👍 / 👎.
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d5cbef3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let useTLS = config.ssl.isEnabled | ||
| let port = config.port > 0 ? config.port : (useTLS ? 8_443 : 8_080) | ||
| return TrinoClientConfig( | ||
| host: config.host.isEmpty ? "localhost" : config.host, | ||
| port: port, | ||
| useTLS: useTLS, | ||
| tls: TrinoSSLMapping.tlsOptions(for: config.ssl), | ||
| user: config.username, | ||
| catalog: config.database.isEmpty ? nil : config.database, | ||
| schema: trimmedField(config.additionalFields["trinoSchema"]), | ||
| timeZone: trimmedField(config.additionalFields["trinoTimeZone"]), | ||
| auth: resolveAuth(config) |
There was a problem hiding this comment.
When a user leaves SSL disabled (the registry default) and uses the default password authentication or JWT option, this constructs an HTTP client configuration while resolveAuth still supplies an Authorization header. The resulting Basic password or bearer token is transmitted in cleartext to the coordinator and any network intermediary; reject this configuration or require TLS before creating the client.
Useful? React with 👍 / 👎.
| func switchDatabase(to database: String) async throws { | ||
| session.setCatalog(database) | ||
| } |
There was a problem hiding this comment.
Switching catalogs leaves session.schema set to the schema selected in the prior catalog. Every later statement POST includes both the new catalog and this stale schema, so if that schema does not exist in the new catalog Trino rejects even SHOW SCHEMAS/sidebar requests before the user can select a replacement. Clear the schema here (or validate it against the target catalog) when the catalog changes.
Useful? React with 👍 / 👎.
| if oldColumn.dataType != newColumn.dataType { | ||
| statements.append(TrinoDDLSQL.setColumnType(qualifiedTable: target, name: newColumn.name, type: newColumn.dataType)) | ||
| } |
There was a problem hiding this comment.
A Structure-tab edit that changes only isNullable produces no statement because this method only checks the name, data type, and comment. Since the plugin exposes the nullable field and emits NOT NULL when creating columns, such edits are silently discarded; generate the connector-supported SET NOT NULL or DROP NOT NULL alteration when the flag changes.
Useful? React with 👍 / 👎.
| private func columnSpec(_ column: PluginColumnDefinition) -> TrinoColumnSpec { | ||
| TrinoColumnSpec( | ||
| name: column.name, | ||
| type: column.dataType, | ||
| nullable: column.isNullable, | ||
| comment: normalizedComment(column.comment) | ||
| ) |
There was a problem hiding this comment.
The plugin advertises a Default Value field, but columnSpec drops PluginColumnDefinition.defaultValue, and TrinoColumnSpec has no place to carry it. Consequently, creating a table or adding a column with a default silently creates a column without that default; include it in the column definition (and handle default changes in modify-column SQL).
Useful? React with 👍 / 👎.
1 participant
Adds Trino as a registry-only, pure-Swift driver plugin. Part of #1906.
Trino speaks a documented HTTP client protocol (
POST /v1/statementthen follownextUriuntil it is gone), so the driver needs no C bridge. It is split into a tested core (TableProTrinoCore) and a thin plugin adapter, matching the existing HTTP drivers.What's included
TableProTrinoCore): the statement lifecycle (POST, pollnextUri, drain), session state threaded from theX-Trino-Set-*response headers, precision-safe value decoding, transient-error backoff (502/503/504 and 429), and cancel viaDELETE. A quote and comment aware statement splitter runs multi-statement DDL correctly (Trino accepts one statement per request).SHOW CATALOGS,SHOW SCHEMAS, and each catalog'sinformation_schema. Materialized views appear in the tree, table row counts come fromSHOW STATS, and table and column comments come frominformation_schemaandsystem.metadata.INSERT/UPDATE/DELETEfrom the grid, keyed on the primary key or the row's other values, with Trino-typed literals (avarcharholding digits stays quoted, a number stays unquoted) andarray/map/rowcolumns left out of the WHERE.nextUriinstead of buffering the whole result set.EXPLAINvariants (Logical, Distributed, IO, Validate, Analyze), a Trino SQL dialect, and a session time zone.DatabaseType.trino, the plugin build target, the registry defaults, the brand icon,build-plugin.yml,release-all-plugins.sh, docs, and CHANGELOG. A small sharedColumnTypeClassifierchange showsarray/map/rowas JSON (also improves ClickHouse and DuckDB nested types).Deliberately deferred, with reasons
supportsTransactions, but Trino DDL is autocommit-only on most connectors, so enabling it would break DDL. Trino runs in autocommit.supportsQueryProgressflag has no working producer anywhere in the app (the ClickHouse path is dead scaffolding). Doing it right means building app-wide progress plumbing, a separate change.X-Trino-Roleis catalog scoped, Kerberos needs SPNEGO, and OAuth 2.0 needs the external browser flow. Left for follow-ups.Testing
TableProTrinoCoreTestscover the poll loop, the bigint precision trap, session threading, retry, error surfacing, the statement splitter, typed literals, DDL and row-edit SQL, streaming, and introspection SQL.swift testpasses.TrinoDriverPluginand the fullTableProapp both build. SwiftLint is clean on the new code.