fix: review comments addressed

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

Author: mbwhite

core/src/main/java/io/substrait/expression/EnumArg.java

@@ -32,10 +32,8 @@ default <R, C extends VisitationContext, E extends Throwable> R accept(
 
   static EnumArg of(SimpleExtension.EnumArgument enumArg, String option) {
     if (!enumArg.options().contains(option)) {
-      LOGGER.warn(
-          String.format(
-              "Function does not accept those arguments %s %s", option, enumArg.options()));
-      return null;
+      throw new IllegalArgumentException(
+          String.format("EnumArg value %s not valid for options: %s", option, enumArg.options()));
     }
     return builder().value(Optional.of(option)).build();
   }

isthmus/src/main/java/io/substrait/isthmus/expression/EnumConverter.java

@@ -4,7 +4,6 @@
 import io.substrait.extension.DefaultExtensionCatalog;
 import io.substrait.extension.SimpleExtension;
 import io.substrait.extension.SimpleExtension.Argument;
-import io.substrait.isthmus.expression.ScalarFunctionConverter.INDEXING;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -78,7 +77,7 @@ public class EnumConverter {
         TimeUnitRange.class);
     calciteEnumMap.put(
         argAnchor(DefaultExtensionCatalog.FUNCTIONS_DATETIME, "extract:req_req_date", 1),
-        INDEXING.class);
+        ExtractIndexing.class);
   }
 
   private static Optional<Enum<?>> constructValue(
@@ -91,9 +90,9 @@ private static Optional<Enum<?>> constructValue(
       return option.get().map(SqlTrimFunction.Flag::valueOf);
     }
 
-    if (cls.isAssignableFrom(INDEXING.class)) {
-      return option.get().map(INDEXING::valueOf);
-    }
+    // ExtractIndexing does not need to be converted here. Calcite
+    // doesn't have the concept of the indexing. It's date
+    // functions are all indexed from 1
 
     return Optional.empty();
   }

isthmus/src/main/java/io/substrait/isthmus/expression/ExtractDateFunctionMapper.java

@@ -3,28 +3,25 @@
 import io.substrait.expression.Expression;
 import io.substrait.expression.FunctionArg;
 import io.substrait.extension.SimpleExtension.ScalarFunctionVariant;
-import io.substrait.isthmus.expression.ScalarFunctionConverter.INDEXING;
-import java.util.ArrayList;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
 import org.apache.calcite.avatica.util.TimeUnitRange;
-import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rex.RexBuilder;
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.rex.RexLiteral;
 import org.apache.calcite.rex.RexNode;
 import org.apache.calcite.sql.fun.SqlStdOperatorTable;
-import org.apache.calcite.sql.type.BasicSqlType;
 import org.apache.calcite.sql.type.SqlTypeName;
 
 /**
  * Custom mapping for the Calcite MONTH/DAY/QUARTER functions.
  *
  * <p>These come from Calcite as 2 argument functions (like YEAR) but in Substrait these functions
- * are 3 agumentes; the additional being if this is a 0 or 1 based value. Calcite is a 1 based value
+ * are 3 arguments; the additional being if this is a 0 or 1 based value. Calcite is a 1 based value
  * in this case.
  *
  * <p>We need to therefore map the MONTH etc functions to a different Substrait function.
@@ -58,12 +55,8 @@ public Optional<SubstraitFunctionMapping> toSubstrait(final RexCall call) {
       return Optional.empty();
     }
 
-    final RelDataType dataType = call.operands.get(1).getType();
-    if (!(dataType instanceof BasicSqlType)) {
-      return Optional.empty();
-    }
-
-    if (!((BasicSqlType) dataType).getSqlTypeName().equals(SqlTypeName.DATE)) {
+    final RexNode dataType = call.operands.get(1);
+    if (!dataType.getType().getSqlTypeName().equals(SqlTypeName.DATE)) {
       return Optional.empty();
     }
 
@@ -74,8 +67,8 @@ public Optional<SubstraitFunctionMapping> toSubstrait(final RexCall call) {
       case MONTH:
       case DAY:
         {
-          final List<RexNode> newOperands = new ArrayList<>(call.operands);
-          newOperands.add(1, RexBuilder.DEFAULT.makeFlag(INDEXING.ONE));
+          final List<RexNode> newOperands = new LinkedList<>(call.operands);
+          newOperands.add(1, RexBuilder.DEFAULT.makeFlag(ExtractIndexing.ONE));
 
           final ScalarFunctionVariant substraitFn =
               this.extractFunctions.get("extract:req_req_date");
@@ -93,7 +86,7 @@ public Optional<List<FunctionArg>> getExpressionArguments(
     String name = expression.declaration().toString();
 
     if ("extract:req_req_date".equals(name)) {
-      final List<FunctionArg> newArgs = new ArrayList<>(expression.arguments());
+      final List<FunctionArg> newArgs = new LinkedList<>(expression.arguments());
       newArgs.remove(1);
 
       return Optional.of(newArgs);

isthmus/src/main/java/io/substrait/isthmus/expression/ExtractIndexing.java

@@ -0,0 +1,11 @@
+package io.substrait.isthmus.expression;
+
+/**
+ * Enum to define the INDEXING property on the date functions.
+ *
+ * <p>Controls if the number used for example in months is 0 or 1 based.
+ */
+public enum ExtractIndexing {
+  ONE,
+  ZERO
+}

isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java

@@ -4,7 +4,6 @@
 import io.substrait.expression.Expression;
 import io.substrait.expression.ExpressionCreator;
 import io.substrait.isthmus.TypeConverter;
-import io.substrait.isthmus.expression.ScalarFunctionConverter.INDEXING;
 import io.substrait.type.Type;
 import java.math.BigDecimal;
 import java.math.RoundingMode;
@@ -89,7 +88,7 @@ public Expression.Literal convert(RexLiteral literal) {
         return ExpressionCreator.bool(n, literal.getValueAs(Boolean.class));
       case CHAR:
         {
-          Comparable val = literal.getValue();
+          Comparable<?> val = literal.getValue();
           if (val instanceof NlsString) {
             NlsString nls = (NlsString) val;
             return ExpressionCreator.fixedChar(n, nls.getValue());
@@ -128,14 +127,10 @@ public Expression.Literal convert(RexLiteral literal) {
       case SYMBOL:
         {
           Object value = literal.getValue();
-          // case TimeUnitRange tur -> string(n, tur.name());
           if (value instanceof NlsString) {
             return ExpressionCreator.string(n, ((NlsString) value).getValue());
           } else if (value instanceof Enum) {
             Enum<?> v = (Enum<?>) value;
-            if (value instanceof INDEXING) {
-              return ExpressionCreator.string(n, v.name());
-            }
 
             Optional<Expression.Literal> r =
                 EnumConverter.canConvert(v)

isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java

@@ -30,11 +30,6 @@ public class ScalarFunctionConverter
    */
   private final List<ScalarFunctionMapper> mappers;
 
-  public enum INDEXING {
-    ONE,
-    ZERO
-  }
-
   public ScalarFunctionConverter(
       List<SimpleExtension.ScalarFunctionVariant> functions, RelDataTypeFactory typeFactory) {
     this(functions, Collections.emptyList(), typeFactory, TypeConverter.DEFAULT);