feat: Set RelBuilder directly in SubstraitRelNodeConverter (#117)

* feat: set RelBuilder directly in SubstraitRelNodeConverter
* refactor: use CatalogReader interface type over concrete implementation
* refactor: use TypeFactory directly rather than through RelOptCluster
* refactor: remove fields only used in constructor
* ci: bump node version to 18
BREAKING CHANGE: SubstraitRelNodeConverter constructor has changed

.github/workflows/pr.yml

@@ -21,7 +21,7 @@ jobs:
           fetch-depth: 0
       - uses: actions/setup-node@v3
         with:
-          node-version: "16"
+          node-version: "18"
       - run: npx commitlint --from=${{ github.event.pull_request.base.sha }} --to=${{ github.sha }} --verbose
   security:
     name: Security validation
@@ -89,6 +89,6 @@ jobs:
           fetch-depth: 0
       - uses: actions/setup-node@v3
         with:
-          node-version: "16"
+          node-version: "18"
       - name: Check current status before next release
         run: ./ci/release/dry_run.sh

isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java

@@ -27,13 +27,14 @@
 import java.util.stream.IntStream;
 import org.apache.calcite.plan.RelOptCluster;
 import org.apache.calcite.plan.RelTraitDef;
-import org.apache.calcite.prepare.CalciteCatalogReader;
+import org.apache.calcite.prepare.Prepare;
 import org.apache.calcite.rel.RelCollations;
 import org.apache.calcite.rel.RelFieldCollation;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.core.AggregateCall;
 import org.apache.calcite.rel.core.JoinRelType;
 import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
 import org.apache.calcite.rex.RexInputRef;
 import org.apache.calcite.rex.RexNode;
 import org.apache.calcite.rex.RexSlot;
@@ -48,57 +49,49 @@
  */
 public class SubstraitRelNodeConverter extends AbstractRelVisitor<RelNode, RuntimeException> {
 
-  private final RelOptCluster relOptCluster;
-  private final CalciteCatalogReader catalogReader;
-
-  private final SimpleExtension.ExtensionCollection extensions;
+  private final RelDataTypeFactory typeFactory;
 
   private final ScalarFunctionConverter scalarFunctionConverter;
-
   private final AggregateFunctionConverter aggregateFunctionConverter;
   private final ExpressionRexConverter expressionRexConverter;
 
   private final RelBuilder relBuilder;
 
   public SubstraitRelNodeConverter(
       SimpleExtension.ExtensionCollection extensions,
-      RelOptCluster relOptCluster,
-      CalciteCatalogReader catalogReader,
-      SqlParser.Config parserConfig) {
-    this.relOptCluster = relOptCluster;
-    this.catalogReader = catalogReader;
-    this.extensions = extensions;
-
-    this.relBuilder =
-        RelBuilder.create(
-            Frameworks.newConfigBuilder()
-                .parserConfig(parserConfig)
-                .defaultSchema(catalogReader.getRootSchema().plus())
-                .traitDefs((List<RelTraitDef>) null)
-                .programs()
-                .build());
+      RelDataTypeFactory typeFactory,
+      RelBuilder relBuilder) {
+    this.typeFactory = typeFactory;
+    this.relBuilder = relBuilder;
 
     this.scalarFunctionConverter =
-        new ScalarFunctionConverter(
-            this.extensions.scalarFunctions(), relOptCluster.getTypeFactory());
+        new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory);
 
     this.aggregateFunctionConverter =
-        new AggregateFunctionConverter(
-            extensions.aggregateFunctions(), relOptCluster.getTypeFactory());
+        new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory);
 
     this.expressionRexConverter =
         new ExpressionRexConverter(
-            relOptCluster.getTypeFactory(), scalarFunctionConverter, aggregateFunctionConverter);
+            typeFactory, scalarFunctionConverter, aggregateFunctionConverter);
   }
 
   public static RelNode convert(
       Rel relRoot,
       RelOptCluster relOptCluster,
-      CalciteCatalogReader calciteCatalogReader,
+      Prepare.CatalogReader catalogReader,
       SqlParser.Config parserConfig) {
+    var relBuilder =
+        RelBuilder.create(
+            Frameworks.newConfigBuilder()
+                .parserConfig(parserConfig)
+                .defaultSchema(catalogReader.getRootSchema().plus())
+                .traitDefs((List<RelTraitDef>) null)
+                .programs()
+                .build());
+
     return relRoot.accept(
         new SubstraitRelNodeConverter(
-            EXTENSION_COLLECTION, relOptCluster, calciteCatalogReader, parserConfig));
+            EXTENSION_COLLECTION, relOptCluster.getTypeFactory(), relBuilder));
   }
 
   @Override
@@ -241,8 +234,7 @@ private AggregateCall fromMeasure(Aggregate.Measure measure) {
         };
 
     SqlAggFunction aggFunction;
-    RelDataType returnType =
-        TypeConverter.convert(relOptCluster.getTypeFactory(), measure.getFunction().getType());
+    RelDataType returnType = TypeConverter.convert(typeFactory, measure.getFunction().getType());
 
     if (operator.get() instanceof SqlAggFunction) {
       aggFunction = (SqlAggFunction) operator.get();