Skip to content

Calcite 1.39.0 will break Isthmus #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mbwhite opened this issue Mar 28, 2025 · 2 comments
Closed

Calcite 1.39.0 will break Isthmus #359

mbwhite opened this issue Mar 28, 2025 · 2 comments

Comments

@mbwhite
Copy link
Contributor

mbwhite commented Mar 28, 2025

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.

@mbwhite
Copy link
Contributor Author

mbwhite commented Mar 31, 2025

resolved in #361

@bvolpato
Copy link
Member

bvolpato commented May 9, 2025

Seems to be resolved at #391, substrait-java was upgraded to Calcite 1.39.0

@bvolpato bvolpato closed this as completed May 9, 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

No branches or pull requests

2 participants