feat(isthmus): new Prepare.CatalogReader based APIs for SQL to/from Substrait #363
Conversation
BREAKING CHANGE: removed function-based table lookup conversion methods
|
This PR removes the following public methods:
and adds 2 new methods that replace their functionality:
The original API was introduced in #26 (along with #33) with the following intent:
At this point though, we expose a number of Calcite objects in our public API in order for users to refine conversion between Substrait and Calcite. Additionally, I would make the point that converting from SQL to Substrait is really 2 conversions:
Given how tightly integrated 1 is with Calcite, I would argue that instead of hiding Calcite details from users it would be more productive to guide them towards using Calcite effectively to parse and unparse SQL. This is part of the aim of #362, which this PR works towards. |
| RelDataTypeFactory factory, CalciteCatalogReader catalog, SqlValidator.Config config) { | ||
| return new Validator(SubstraitOperatorTable.INSTANCE, catalog, factory, config); | ||
| RelDataTypeFactory factory, | ||
| SqlValidatorCatalogReader validatorCatalog, |
There was a problem hiding this comment.
Calcite only requires a SqlValidatorCatalogReader, not a full CalciteCatalogReader when constructing a validator.
| } | ||
| } | ||
| return Pair.of(validator, catalogReader); | ||
| return catalogReader; |
There was a problem hiding this comment.
I've decoupled the creation of the schema and the creation of the validator. The validator is not need as part of creating the schema. The validator can be created after with the schema we produce here. Splitting this apart makes this more explicit. It also simplifies some future changes I have in mind.
These methods are all internal.
| var pair = registerCreateTables(tables); | ||
| return executeInner(sql, factory, pair.left, pair.right); | ||
| CalciteCatalogReader catalogReader = registerCreateTables(tables); | ||
| SqlValidator validator = Validator.create(factory, catalogReader, SqlValidator.Config.DEFAULT); |
There was a problem hiding this comment.
As indicated above, we now create the schema first and create validator from the schema. This works because the validator is not needed to validate the schema, but rather the query we are parsing.
|
|
||
| public SubstraitToSql() { | ||
| super(FEATURES_DEFAULT); | ||
| CalciteSchema rootSchema = CalciteSchema.createRootSchema(false); |
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class TableLookupTest extends PlanTestBase { |
There was a problem hiding this comment.
This test is no longer needed as this functionality is no longer supported.
|
Tagging @nielspardon @mbwhite as you've both been poking at the SQL conversion side of things. Tagging @rymurr, @jinfengni as you added the original APIs that are being removed. |
vbarua
changed the title
|
Hi @vbarua agree with what you've proposed here; I understand the original intent, and the logic there but agree things have moved on as well. As found with Calcite 1.39, there's (another) lazy mechanism to get table schema information; not sure how the |
The usage of |
| return executeInner(sql, validator, catalogReader); | ||
| } | ||
|
|
||
| public Plan execute(String sql, String name, Schema schema) throws SqlParseException { |
There was a problem hiding this comment.
This API is somewhat redundant when we add execute(String sql, Prepare.CatalogReader catalogReader) as users can easily convert the Schema to a Prepare.CatalogReader satisfying instance themselves.
Choosing to leave it in for now.
|
I can reapply the Calcite 1.39 changes after this - if they are needed at all after the API change. |
BREAKING CHANGE: removed function-based table lookup conversion methods