-
Notifications
You must be signed in to change notification settings - Fork 92
Description
Calcite 1.39.0 has a change to better support large numbers of schemas. see https://issues.apache.org/jira/browse/CALCITE-6728; this has resulted in a change to the internals for the org.apache.calcite.jdbc.SimpleCalicateSchema; this class is extended by LookupCalciteSchema in the Isthmus codebase. As this subclass has removed and altered its protected methods, fails Ishtmus fails to compile (though if you run without recompiling, errors relating to missing tables abound)
(aside: there is an argument that the protected methods of non-final classes should be considered an API and not changed; however, from a best-practice point of view, irrespective of any changes, it would be better not to adopt the pattern LookupCalciteSchema is following. i.e. having to be in the same package as a library you are dependent on and subclassing.)
This LookupCalciteSchema class is used extensively by the conversions from Substrait to Calcite & SQL.
I can resolve this by changing toSchema(Rel rel) method in the SubstraitToCalcite codebase - something similar to this
protected CalciteSchema toSchema(Rel rel) {
Map<List<String>, NamedStruct> tableMap = NamedStructGatherer.gatherTables(rel);
Map<String, Table> tables = tableMap.entrySet().stream().map(entry -> {
var id = entry.getKey();
var table = entry.getValue();
var value = new SqlConverterBase.DefinedTable(
id.get(id.size() - 1),
typeFactory,
typeConverter.toCalcite(typeFactory, table.struct(), table.names()));
return Map.entry(id.get(id.size() - 1), value);
}).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
var schema = new AbstractSchema(){
@Override
protected Map<String, Table> getTableMap() {
return tables;
}
};
return CalciteSchema.createRootSchema(false, false, "", schema);
}
This gives a 99% pass rate without the 4 tests failing in LookupTableTest - however as this test is specifically relying on the deferred lookup of the table names; which (ironically) is what the new feature in calcite is intended to support. This knocks a couple more tests off;
What's left are two namespace tests... where the namespace tablename org.example.FOO say is coming out in calcite as just being FOO....
UPDATE: solution has been found!
Tidying up the solution ahead of a PR.