feat: define how TIME data is formatted (#7674)

* feat: add TIME and DATE types * Add the DateType and TimeType files * feat: define how TIME data is printed * remove DateLiteral * revert literal terms for now * remove unused import * add positive time literal tests
confluentinc · Jun 16, 2021 · 7718955 · 7718955
1 parent 8f2d799
commit 7718955
Show file tree

Hide file tree

Showing 12 changed files with 43 additions and 32 deletions.
diff --git a/ksqldb-engine-common/src/main/java/io/confluent/ksql/schema/ksql/SqlTimestamps.java b/ksqldb-engine-common/src/main/java/io/confluent/ksql/schema/ksql/SqlTimestamps.java
@@ -17,12 +17,14 @@
 
 import io.confluent.ksql.util.KsqlConstants;
 import io.confluent.ksql.util.timestamp.PartialStringToTimestampParser;
+import java.sql.Time;
 import java.sql.Timestamp;
+import java.time.LocalTime;
 import java.time.ZoneId;
 import java.time.format.DateTimeFormatter;
 
 /**
- * Helpers for working with Sql {@code TIMESTAMP}.
+ * Helpers for working with SQL time types.
  */
 public final class SqlTimestamps {
 
@@ -49,4 +51,8 @@ public static String formatTimestamp(final Timestamp timestamp) {
         .withZone(ZoneId.of("Z"))
         .format(timestamp.toInstant());
   }
+
+  public static String formatTime(final Time time) {
+    return LocalTime.ofSecondOfDay(time.getTime() / 1000).toString();
+  }
 }
diff --git a/ksqldb-engine-common/src/test/java/io/confluent/ksql/schema/ksql/SqlTimestampsTest.java b/ksqldb-engine-common/src/test/java/io/confluent/ksql/schema/ksql/SqlTimestampsTest.java
@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertThrows;
 
 import io.confluent.ksql.util.KsqlException;
+import java.sql.Time;
 import java.sql.Timestamp;
 import org.junit.Test;
 
@@ -40,4 +41,10 @@ public void shouldNotParseTimestamp() {
   public void shouldFormatTimestamp() {
     assertThat(SqlTimestamps.formatTimestamp(new Timestamp(1552816800000L)), is("2019-03-17T10:00:00.000"));
   }
+
+  @Test
+  public void shouldFormatTime() {
+    assertThat(SqlTimestamps.formatTime(new Time(1000)), is("00:00:01"));
+    assertThat(SqlTimestamps.formatTime(new Time(1005)), is("00:00:01"));
+  }
 }
diff --git a/ksqldb-engine/src/test/java/io/confluent/ksql/engine/rewrite/ExpressionTreeRewriterTest.java b/ksqldb-engine/src/test/java/io/confluent/ksql/engine/rewrite/ExpressionTreeRewriterTest.java
@@ -76,6 +76,7 @@
 import io.confluent.ksql.util.KsqlParserTestUtil;
 import io.confluent.ksql.util.MetaStoreFixture;
 import java.math.BigDecimal;
+import java.sql.Time;
 import java.sql.Timestamp;
 import java.util.List;
 import java.util.Optional;
@@ -99,7 +100,7 @@ public class ExpressionTreeRewriterTest {
       new StringLiteral("abcd"),
       new NullLiteral(),
       new DecimalLiteral(BigDecimal.ONE),
-      new TimeLiteral("00:00:00"),
+      new TimeLiteral(new Time(1000L)),
       new TimestampLiteral(new Timestamp(0))
   );
 

diff --git a/ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java b/ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java
@@ -339,10 +339,10 @@ public Pair<String, SqlType> visitTimestampLiteral(
 
     @Override
     public Pair<String, SqlType> visitTimeLiteral(
-        final TimeLiteral timeLiteral,
+        final TimeLiteral node,
         final Context context
     ) {
-      return visitUnsupported(timeLiteral);
+      return new Pair<>(node.toString(), SqlTypes.TIME);
     }
 
     @Override

diff --git a/...n/src/main/java/io/confluent/ksql/execution/expression/formatter/ExpressionFormatter.java b/...n/src/main/java/io/confluent/ksql/execution/expression/formatter/ExpressionFormatter.java
@@ -191,7 +191,7 @@ public String visitDecimalLiteral(final DecimalLiteral node, final Context conte
 
     @Override
     public String visitTimeLiteral(final TimeLiteral node, final Context context) {
-      return "TIME '" + node.getValue() + "'";
+      return SqlTimestamps.formatTime(node.getValue());
     }
 
     @Override

diff --git a/ksqldb-execution/src/main/java/io/confluent/ksql/execution/expression/tree/TimeLiteral.java b/ksqldb-execution/src/main/java/io/confluent/ksql/execution/expression/tree/TimeLiteral.java
@@ -19,26 +19,27 @@
 
 import com.google.errorprone.annotations.Immutable;
 import io.confluent.ksql.parser.NodeLocation;
+import java.sql.Time;
 import java.util.Objects;
 import java.util.Optional;
 
 @Immutable
 public class TimeLiteral extends Literal {
 
-  private final String value;
+  private final long value;
 
-  public TimeLiteral(final String value) {
+  public TimeLiteral(final Time value) {
     this(Optional.empty(), value);
   }
 
-  public TimeLiteral(final Optional<NodeLocation> location, final String value) {
+  public TimeLiteral(final Optional<NodeLocation> location, final Time value) {
     super(location);
-    this.value = requireNonNull(value, "value");
+    this.value = requireNonNull(value, "value").getTime();
   }
 
   @Override
-  public String getValue() {
-    return value;
+  public Time getValue() {
+    return new Time(value);
   }
 
   @Override
@@ -61,6 +62,6 @@ public boolean equals(final Object o) {
 
   @Override
   public int hashCode() {
-    return value.hashCode();
+    return Long.hashCode(value);
   }
 }
diff --git a/ksqldb-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java b/ksqldb-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java
@@ -573,7 +573,8 @@ public Void visitType(final Type type, final Context context) {
     public Void visitTimeLiteral(
         final TimeLiteral timeLiteral, final Context context
     ) {
-      throw VisitorUtil.unsupportedOperation(this, timeLiteral);
+      context.setSqlType(SqlTypes.TIME);
+      return null;
     }
 
     @Override

diff --git a/ksqldb-execution/src/main/java/io/confluent/ksql/execution/util/Literals.java b/ksqldb-execution/src/main/java/io/confluent/ksql/execution/util/Literals.java
@@ -23,16 +23,20 @@
 import io.confluent.ksql.execution.expression.tree.Literal;
 import io.confluent.ksql.execution.expression.tree.LongLiteral;
 import io.confluent.ksql.execution.expression.tree.StringLiteral;
+import io.confluent.ksql.execution.expression.tree.TimeLiteral;
 import io.confluent.ksql.execution.expression.tree.TimestampLiteral;
 import io.confluent.ksql.schema.ksql.types.SqlBaseType;
 import java.math.BigDecimal;
+import java.sql.Time;
 import java.sql.Timestamp;
 import java.util.function.Function;
 
 /**
  * Literal helpers
  */
+// CHECKSTYLE_RULES.OFF: ClassDataAbstractionCoupling
 public final class Literals {
+  // CHECKSTYLE_RULES.ON: ClassDataAbstractionCoupling
 
   private static final ImmutableMap<SqlBaseType, Function<Object, ? extends Literal>> FACTORIES =
       ImmutableMap.<SqlBaseType, Function<Object, ? extends Literal>>builder()
@@ -42,6 +46,7 @@ public final class Literals {
           .put(SqlBaseType.DECIMAL, v -> new DecimalLiteral((BigDecimal) v))
           .put(SqlBaseType.DOUBLE, v -> new DoubleLiteral((Double) v))
           .put(SqlBaseType.STRING, v -> new StringLiteral((String) v))
+          .put(SqlBaseType.TIME, v -> new TimeLiteral((Time) v))
           .put(SqlBaseType.TIMESTAMP, v -> new TimestampLiteral((Timestamp) v))
           .build();
 

diff --git a/ksqldb-execution/src/test/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitorTest.java b/ksqldb-execution/src/test/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitorTest.java
@@ -83,6 +83,7 @@
 import io.confluent.ksql.schema.ksql.types.SqlTypes;
 import io.confluent.ksql.util.KsqlConfig;
 import java.math.BigDecimal;
+import java.sql.Time;
 import java.util.Collections;
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
@@ -1150,12 +1151,8 @@ public void shouldThrowOnSimpleCase() {
   }
 
   @Test
-  public void shouldThrowOnTimeLiteral() {
-    // When:
-    assertThrows(
-        UnsupportedOperationException.class,
-        () -> sqlToJavaVisitor.process(new TimeLiteral("TIME '00:00:00'"))
-    );
+  public void shouldProcessTimeLiteral() {
+    assertThat(sqlToJavaVisitor.process(new TimeLiteral(new Time(1000))), is("00:00:01"));
   }
 
   private void givenUdf(

diff --git a/...c/test/java/io/confluent/ksql/execution/expression/formatter/ExpressionFormatterTest.java b/...c/test/java/io/confluent/ksql/execution/expression/formatter/ExpressionFormatterTest.java
@@ -17,6 +17,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.mock;
 
 import com.google.common.collect.ImmutableList;
@@ -70,6 +71,7 @@
 import io.confluent.ksql.schema.ksql.types.SqlTypes;
 import io.confluent.ksql.schema.utils.FormatOptions;
 import java.math.BigDecimal;
+import java.sql.Time;
 import java.sql.Timestamp;
 import java.util.Collections;
 import java.util.Optional;
@@ -169,7 +171,7 @@ public void shouldFormatDecimalLiteral() {
 
   @Test
   public void shouldFormatTimeLiteral() {
-    assertThat(ExpressionFormatter.formatExpression(new TimeLiteral("17/9/2017")), equalTo("TIME '17/9/2017'"));
+    assertThat(ExpressionFormatter.formatExpression(new TimeLiteral(new Time(10000))), equalTo("00:00:10"));
   }
 
   @Test

diff --git a/...b-execution/src/test/java/io/confluent/ksql/execution/util/ExpressionTypeManagerTest.java b/...b-execution/src/test/java/io/confluent/ksql/execution/util/ExpressionTypeManagerTest.java
@@ -51,7 +51,6 @@
 import io.confluent.ksql.execution.expression.tree.InListExpression;
 import io.confluent.ksql.execution.expression.tree.InPredicate;
 import io.confluent.ksql.execution.expression.tree.IntegerLiteral;
-import io.confluent.ksql.execution.expression.tree.IntervalUnit;
 import io.confluent.ksql.execution.expression.tree.LambdaFunctionCall;
 import io.confluent.ksql.execution.expression.tree.LambdaVariable;
 import io.confluent.ksql.execution.expression.tree.LikePredicate;
@@ -90,8 +89,8 @@
 import io.confluent.ksql.schema.ksql.types.SqlType;
 import io.confluent.ksql.schema.ksql.types.SqlTypes;
 import io.confluent.ksql.util.KsqlException;
+import java.sql.Time;
 import java.util.Optional;
-import java.util.concurrent.TimeUnit;
 import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Rule;
@@ -1059,12 +1058,8 @@ public void shouldFailIfDefaultHasDifferentTypeToWhen() {
   }
 
   @Test
-  public void shouldThrowOnTimeLiteral() {
-    // When:
-    assertThrows(
-        UnsupportedOperationException.class,
-        () -> expressionTypeManager.getExpressionSqlType(new TimeLiteral("TIME '00:00:00'"))
-    );
+  public void shouldProcessTimeLiteral() {
+    assertThat(expressionTypeManager.getExpressionSqlType(new TimeLiteral(new Time(1000))), is(SqlTypes.TIME));
   }
 
   @Test

diff --git a/ksqldb-parser/src/main/java/io/confluent/ksql/parser/AstBuilder.java b/ksqldb-parser/src/main/java/io/confluent/ksql/parser/AstBuilder.java
@@ -55,7 +55,6 @@
 import io.confluent.ksql.execution.expression.tree.SimpleCaseExpression;
 import io.confluent.ksql.execution.expression.tree.StringLiteral;
 import io.confluent.ksql.execution.expression.tree.SubscriptExpression;
-import io.confluent.ksql.execution.expression.tree.TimeLiteral;
 import io.confluent.ksql.execution.expression.tree.UnqualifiedColumnReferenceExp;
 import io.confluent.ksql.execution.expression.tree.WhenClause;
 import io.confluent.ksql.execution.windows.HoppingWindowExpression;
@@ -1269,9 +1268,6 @@ public Node visitTypeConstructor(final SqlBaseParser.TypeConstructorContext cont
       final String value = ParserUtil.unquote(context.STRING().getText(), "'");
       final Optional<NodeLocation> location = getLocation(context);
 
-      if (type.equals("TIME")) {
-        return new TimeLiteral(location, value);
-      }
       if (type.equals("DECIMAL")) {
         return new DecimalLiteral(location, new BigDecimal(value));
       }