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

Removes unused SPI APIs and makes connector instantiate catalogs #1588

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

RCHowell
Copy link
Member

Relevant Issues

#1496

Description

This PR is a cleanup of extra and unused SPI APIs, and it is the final modifications to the connector/catalog components of the SPI. We have effectively moved from Trino's model of plugins+connector to a "catalog" interface like Iceberg and Calcite.

Other Information

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

github-actions bot commented Sep 20, 2024

CROSS-ENGINE Conformance Report ❌

BASE (LEGACY-F1748FA) TARGET (EVAL-F1748FA) +/-
% Passing 89.69% 94.20% 4.51% ✅
Passing 5288 5554 266 ✅
Failing 608 75 -533 ✅
Ignored 0 267 267 🔶
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: f1748fa
  • Base Engine: LEGACY
  • Target Commit: f1748fa
  • Target Engine: EVAL

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing Tests. ❌
  • Passing in both: 5019
  • Failing in both: 73
  • PASSING in BASE but now FAILING in TARGET: 2
  • FAILING in BASE but now PASSING in TARGET: 535

Now Failing Tests ❌

The following 2 test(s) were previously PASSING in BASE but are now FAILING in TARGET:

Click here to see
  1. MYSQL_SELECT_29, compileOption: PERMISSIVE
  2. MYSQL_SELECT_29, compileOption: LEGACY

Now Passing Tests

535 test(s) were previously failing in BASE (LEGACY-F1748FA) but now pass in TARGET (EVAL-F1748FA). Before merging, confirm they are intended to pass.

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

CROSS-COMMIT-LEGACY Conformance Report ✅

BASE (LEGACY-3281F43) TARGET (LEGACY-F1748FA) +/-
% Passing 89.67% 89.69% 0.02% ✅
Passing 5287 5288 1 ✅
Failing 609 608 -1 ✅
Ignored 0 0 0 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: 3281f43
  • Base Engine: LEGACY
  • Target Commit: f1748fa
  • Target Engine: LEGACY

Result Details

  • Passing in both: 5287
  • Failing in both: 608
  • PASSING in BASE but now FAILING in TARGET: 0
  • FAILING in BASE but now PASSING in TARGET: 1

Now Passing Tests

The following 1 test(s) were previously FAILING in BASE but are now PASSING in TARGET. Before merging, confirm they are intended to pass:

Click here to see
  1. MYSQL_SELECT_29, compileOption: PERMISSIVE

CROSS-COMMIT-EVAL Conformance Report ✅

BASE (EVAL-3281F43) TARGET (EVAL-F1748FA) +/-
% Passing 94.20% 94.20% 0.00% ✅
Passing 5554 5554 0 ✅
Failing 75 75 0 ✅
Ignored 267 267 0 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: 3281f43
  • Base Engine: EVAL
  • Target Commit: f1748fa
  • Target Engine: EVAL

Result Details

  • Passing in both: 5554
  • Failing in both: 75
  • PASSING in BASE but now FAILING in TARGET: 0
  • FAILING in BASE but now PASSING in TARGET: 0

* @param name Catalog name
* @return
*/
public fun getCatalog(name: String): Catalog
Copy link
Member

Choose a reason for hiding this comment

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

Will this ever be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the same as 0.14 with the null config. I've just added the override manually.

public fun create(catalogName: String, config: StructElement? = null): Connector

https://github.com/partiql/partiql-lang-kotlin/blob/main/partiql-spi/src/main/kotlin/org/partiql/spi/connector/Connector.kt#L50

* @param context Context is an arbitrary object
* @return
*/
public fun getCatalog(name: String, context: Context): Catalog
Copy link
Member

Choose a reason for hiding this comment

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

Given that a name is carried by the Catalog itself, it might not be necessary to pass it here (or not necessary to keep it in the Catalog). It may present the opportunity for mismatching names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Think of this being looking up a catalog implementation (or instantiating if it does not exist). I see this as no different than the Catalog's getTable(name: Name): Table which also presents the opportunity for mismatching names.

Now .. that would be a bad implementation which is kind of like returning the wrong table given the name. We can't stop a customer from implementing something incorrect.

@RCHowell RCHowell merged commit 3fb4b3e into v1 Sep 24, 2024
14 checks passed
@RCHowell RCHowell deleted the v1-spi-cleanup branch September 24, 2024 18:18
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