refactor(examples): simplify ToOptimizedSql example

[vbarua]

Substrait-to-Calcite conversion is now done via SubstraitToCalcite directly.
Optimization and SQL emission are handled as plain Calcite operations by the caller,
making the separation of concerns explicit.

[vbarua]

@mbwhite small suggestion for your ToOptimizedSql.

I think that trying to extend Isthmus components to handle more of Calcite configurability is a losing battle. There are many many ways that people want to use Calcite. I think a cleaner approach overall is to push users towards composing Isthmus and Calcite. They can use Isthmus utilities to help with converting from Substrait to Calcite, and then use standard Calcite code to optimize plans and generate SQL.

[mbwhite]

Hi @vbarua I agree with you about Isthmus and Calcite; the Isthmus API is now a lot better. The flow and separation of concerns I think are in a good place now.

Just on the example here - it wasn't intended to be an extension to Isthmus more an example of how in production the code. could be componentised. Drawing attention to a pattern the ConverterProvider enables. I can't personally see why that would be an anti-pattern but might have missed something. We can make the class final if subclassing is laying problems in wait for the future.

[bestbeforetoday]

I like this change and the separation of Calcite to/from Substrait and Calcite to/from SQL.

I think @mbwhite raises an interesting point about subclassing and finality. My understanding is that ConverterProvider is intended to be subclassed, just perhaps not for the purposes it was done for here. Isthmus already provides two subclass implementations: DynamicConverterProvider and AutomaticDynamicFunctionMappingConverterProvider.

I notice that classes like SubstraitToCalcite seem to be intended for subclassing too, being non-final and with protected members. I am not sure this is ideal. Customization of behavior is supposed to be done with a supplied ConverterProvider rather than subclassing. Perhaps SubstraitToCalcite (and similar classes) should be final. What do you think @vbarua?

[vbarua]

My understanding is that ConverterProvider is intended to be subclassed, just perhaps not for the purposes it was done for here.

My intent was that it would be subclassed to modify conversion behaviours from Substrait to Calcite. I don't think of applying optimization as part of that conversion because you don't actually need any code from Isthmus to do it. Optimization is the domain of Calcite, and I want to lightly encourage users to think of Isthmus as the conversion library, and Calcite as the optimization library. One way to encourage this is to have our examples follow that pattern.

In that past, we had more hooks and toggles in the API that would configure some aspect of Calcite behavior internally. Inevitable we needed to add more toggles to tweak more things, and that felt like a losing battle overall. Instead of exposing Calcite stuff incrementally, I moved the library roughly in the direction of:

1. Utilizing Calcite configs directly when it made sense via the ConverterProvider
2. Splitting Isthmus specific functionality apart from Calcite functionality and pushing towards a pipeline processing model of SQL -> Calcite -> Substrait -> Calcite -> SQL where each individual step can be it's own class roughly, allowing for easier customization.

I notice that classes like SubstraitToCalcite seem to be intended for subclassing too, being non-final and with protected members. I am not sure this is ideal. Customization of behavior is supposed to be done with a supplied ConverterProvider rather than subclassing. Perhaps SubstraitToCalcite (and similar classes) should be final.

Hmmm. Before the introduction of the ConverterProvider, subclassing the SubstraitToCalcite was how conversion behaviour was customized. It may not be needed anymore, but I'm not sure. I know that we have code internally that still subclasses it, but it may not be needed anymore. It would take me a bit of time to chase down an answer to that.

[bestbeforetoday]

Looks good to me!