Skip to content

Conversation

@mbwhite
Copy link
Contributor

@mbwhite mbwhite commented Apr 9, 2025

(replacement for PR #361

As a result of an (accidental) upgrade Calcite 1.39; conversions via Isthmus started to report that table names where unknown;

Isthmus had an external API that allows tables to be lookup via calls to a function - dynamic or 'lazy' lookup. A special subclass of the CalciteSimpleSchema was created to support this. This special subclass had to be in the same package as the CalciteSimpleSchema as it took advantage of being able to override a number of protected methods.

Calcite 1.39.0 introduced similar support for 'lazy' table lookups; that implementation, though changed the protected methods sufficiently Isthmus now failed (indeed rebuilding the code fails to compile).

This PR removes the special subclass, and allows Ishtmus to work as before; with the work down to remove the external function based APIs there was only 1 location left where this special schema class was used.

This has been altered to use a new class that implements a Calcite Schema; it does it in a much simpler way than the previously

Signed-off-by: MBWhite <whitemat@uk.ibm.com>
@mbwhite mbwhite marked this pull request as ready for review April 9, 2025 09:28
@vbarua
Copy link
Member

vbarua commented Apr 19, 2025

I have an alternative proposal for building CalciteSchemas in #391.

That one also handles nested schemas better, which the existing LookupCalciteSchema didn't really do.

@bestbeforetoday
Copy link
Contributor

@vbarua Calcite 1.39.0 includes some fixes that enable several additional TPC-DS queries to be successfully handled by Isthmus. I have locally tested both this pull request and your pull request (with an additional update of the Calcite version), and both enable support of the additional TPC-DS queries.

Since this change appears to be roughly a subset of your change, would it make sense to merge this and then base your additional changes on top? Or if you prefer to go straight to your change, any outlook on when it can be merged? Is it just waiting on a reviewer to get to it?

@vbarua
Copy link
Member

vbarua commented Apr 29, 2025

Or if you prefer to go straight to your change, any outlook on when it can be merged?

I would prefer to do this to avoid merging code which will then just be deleted or heavily modified. I've asked for a review in the Slack to hopefully move that PR along.

@mbwhite
Copy link
Contributor Author

mbwhite commented May 6, 2025

Closing this as the work has been done in other PRs.

@mbwhite mbwhite closed this May 6, 2025
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.

3 participants