Skip to content

Calcite 1.39.0 will break Isthmus #359

@mbwhite

Description

@mbwhite

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions