fix isthmus

substrait-io · Aug 21, 2024 · 8e4ef88 · 8e4ef88
1 parent 2f9c944
commit 8e4ef88
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 13 deletions.
diff --git a/core/src/main/java/io/substrait/expression/AbstractExpressionVisitor.java b/core/src/main/java/io/substrait/expression/AbstractExpressionVisitor.java
@@ -94,6 +94,11 @@ public OUTPUT visit(Expression.IntervalDayLiteral expr) throws EXCEPTION {
     return visitFallback(expr);
   }
 
+  @Override
+  public OUTPUT visit(Expression.IntervalCompoundLiteral expr) throws EXCEPTION {
+    return visitFallback(expr);
+  }
+
   @Override
   public OUTPUT visit(Expression.UUIDLiteral expr) throws EXCEPTION {
     return visitFallback(expr);

diff --git a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java
@@ -341,8 +341,8 @@ public Expression.Literal from(io.substrait.proto.Expression.Literal literal) {
           literal.getIntervalYearToMonth().getYears(),
           literal.getIntervalYearToMonth().getMonths());
       case INTERVAL_DAY_TO_SECOND -> {
-        // Handle deprecated version that doesn't provide precision and that uses microseconds instead
-        // of subseconds, for backwards compatibility
+        // Handle deprecated version that doesn't provide precision and that uses microseconds
+        // instead of subseconds, for backwards compatibility
         int precision =
             literal.getIntervalDayToSecond().hasPrecision()
                 ? literal.getIntervalDayToSecond().getPrecision()

diff --git a/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/TypeConverter.java
@@ -102,7 +102,7 @@ private Type toSubstrait(RelDataType type, List<String> names) {
           INTERVAL_HOUR_SECOND,
           INTERVAL_MINUTE,
           INTERVAL_MINUTE_SECOND,
-          INTERVAL_SECOND -> creator.INTERVAL_DAY;
+          INTERVAL_SECOND -> creator.intervalDay(type.getScale());
       case VARBINARY -> creator.BINARY;
       case BINARY -> creator.fixedBinary(type.getPrecision());
       case MAP -> {

diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java
@@ -229,17 +229,17 @@ public RexNode visit(Expression.IntervalYearLiteral expr) throws RuntimeExceptio
         new BigDecimal(expr.years() * 12 + expr.months()), YEAR_MONTH_INTERVAL);
   }
 
-  private static final long MICROS_IN_DAY = TimeUnit.DAYS.toMicros(1);
+  private static final long MILLIS_IN_DAY = TimeUnit.DAYS.toMillis(1);
 
   @Override
   public RexNode visit(Expression.IntervalDayLiteral expr) throws RuntimeException {
+    long milliseconds =
+        expr.precision() > 3
+            ? (expr.subseconds() / (int) Math.pow(10, expr.precision() - 3))
+            : (expr.subseconds() * (int) Math.pow(10, 3 - expr.precision()));
     return rexBuilder.makeIntervalLiteral(
         // Current Calcite behavior is to store milliseconds since Epoch
-        // microseconds version: new BigDecimal(expr.days() * MICROS_IN_DAY + expr.seconds() *
-        // 100000L + expr.microseconds()), DAY_SECOND_INTERVAL);
-        new BigDecimal(
-            (expr.days() * MICROS_IN_DAY + expr.seconds() * 1_000_000L + expr.microseconds())
-                / 1000L),
+        new BigDecimal((expr.days() * MILLIS_IN_DAY + expr.seconds() * 1_000L + milliseconds)),
         DAY_SECOND_INTERVAL);
   }
 

diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/IgnoreNullableAndParameters.java b/isthmus/src/main/java/io/substrait/isthmus/expression/IgnoreNullableAndParameters.java
@@ -85,7 +85,14 @@ public Boolean visit(Type.IntervalYear type) {
 
   @Override
   public Boolean visit(Type.IntervalDay type) {
-    return typeToMatch instanceof Type.IntervalDay;
+    return typeToMatch instanceof Type.IntervalDay
+        || typeToMatch instanceof ParameterizedType.IntervalDay;
+  }
+
+  @Override
+  public Boolean visit(Type.IntervalCompound type) {
+    return typeToMatch instanceof Type.IntervalCompound
+        || typeToMatch instanceof ParameterizedType.IntervalCompound;
   }
 
   @Override
@@ -171,6 +178,18 @@ public Boolean visit(ParameterizedType.Decimal expr) throws RuntimeException {
     return typeToMatch instanceof Type.Decimal || typeToMatch instanceof ParameterizedType.Decimal;
   }
 
+  @Override
+  public Boolean visit(ParameterizedType.IntervalDay expr) throws RuntimeException {
+    return typeToMatch instanceof Type.IntervalDay
+        || typeToMatch instanceof ParameterizedType.IntervalDay;
+  }
+
+  @Override
+  public Boolean visit(ParameterizedType.IntervalCompound expr) throws RuntimeException {
+    return typeToMatch instanceof Type.IntervalCompound
+        || typeToMatch instanceof ParameterizedType.IntervalCompound;
+  }
+
   @Override
   public Boolean visit(ParameterizedType.PrecisionTimestamp expr) throws RuntimeException {
     return typeToMatch instanceof Type.PrecisionTimestamp

diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java
@@ -172,6 +172,7 @@ public Expression.Literal convert(RexLiteral literal) {
           INTERVAL_MINUTE_SECOND,
           INTERVAL_SECOND -> {
         // we need to convert to microseconds.
+        // TODO: don't need to anymore
         int scale = literal.getType().getScale();
         var intervalLength = literal.getValueAs(BigDecimal.class).longValue();
         var adjustedLength =
@@ -182,7 +183,7 @@ public Expression.Literal convert(RexLiteral literal) {
         var totalMicroseconds = adjustedLength - days * MICROS_IN_DAY;
         var seconds = totalMicroseconds / 1_000_000;
         var microseconds = totalMicroseconds - 1_000_000 * seconds;
-        yield intervalDay(n, (int) days, (int) seconds, (int) microseconds);
+        yield intervalDay(n, (int) days, (int) seconds, microseconds, 6 /* micros */);
       }
 
       case ROW -> {

diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteLiteralTest.java
@@ -211,7 +211,7 @@ void tIntervalMillisecond() {
                 org.apache.calcite.avatica.util.TimeUnit.SECOND,
                 3,
                 SqlParserPos.ZERO));
-    var intervalDaySecondExpr = intervalDay(false, 3, 5 * 3600 + 7 * 60 + 9, 500_000);
+    var intervalDaySecondExpr = intervalDay(false, 3, 5 * 3600 + 7 * 60 + 9, 500_000, 6);
     bitest(intervalDaySecondExpr, intervalDaySecond);
   }
 

diff --git a/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java b/isthmus/src/test/java/io/substrait/isthmus/CalciteTypeTest.java
@@ -118,7 +118,7 @@ void intervalYear(boolean nullable) {
   @ValueSource(booleans = {true, false})
   void intervalDay(boolean nullable) {
     testType(
-        Type.withNullability(nullable).INTERVAL_DAY,
+        Type.withNullability(nullable).intervalDay(6),
         type.createSqlIntervalType(SubstraitTypeSystem.DAY_SECOND_INTERVAL),
         nullable);
   }