apilloud commented on a change in pull request #12054:
URL: https://github.com/apache/beam/pull/12054#discussion_r445832856
##########
File path:
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamComplexTypeTest.java
##########
@@ -452,21 +412,32 @@ public void testNullDatetimeFields() {
.addNullableField("year_with_null", FieldType.INT64)
.addField("mm", FieldType.INT64)
.addNullableField("month_with_null", FieldType.INT64)
- .addField("time_with_hour_added", FieldType.DATETIME)
Review comment:
If I remember right, `FieldType.DATETIME` is the SQL type of
`TIMESTAMP`. Looks like all the test cases for `TIMESTAMP` have been dropped?
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
##########
@@ -315,34 +317,38 @@ private static Expression castOutput(Expression value,
FieldType toType) {
private static Expression castOutputTime(Expression value, FieldType toType)
{
Expression valueDateTime = value;
- // First, convert to millis (except for DATE type)
+ // Convert TIMESTAMP to joda Instant
Review comment:
nit: Can you move each of these comments into the appropriate if block?
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
##########
@@ -315,34 +317,38 @@ private static Expression castOutput(Expression value,
FieldType toType) {
private static Expression castOutputTime(Expression value, FieldType toType)
{
Expression valueDateTime = value;
- // First, convert to millis (except for DATE type)
+ // Convert TIMESTAMP to joda Instant
+ // Convert DATE to LocalDate
+ // Convert TIME to LocalTime
if (CalciteUtils.TIMESTAMP.typesEqual(toType)
|| CalciteUtils.NULLABLE_TIMESTAMP.typesEqual(toType)) {
if (value.getType() == java.sql.Timestamp.class) {
valueDateTime =
Expressions.call(BuiltInMethod.TIMESTAMP_TO_LONG.method, valueDateTime);
}
+ valueDateTime = Expressions.new_(Instant.class, valueDateTime);
} else if (CalciteUtils.TIME.typesEqual(toType)
|| CalciteUtils.NULLABLE_TIME.typesEqual(toType)) {
if (value.getType() == java.sql.Time.class) {
valueDateTime = Expressions.call(BuiltInMethod.TIME_TO_INT.method,
valueDateTime);
+ } else if (value.getType() == Long.class) {
Review comment:
nit: There is no test case for this.
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
##########
@@ -431,11 +437,11 @@ private static Expression value(
private static Expression value(Expression value, Schema.FieldType type) {
if (type.getTypeName().isLogicalType()) {
String logicalId = type.getLogicalType().getIdentifier();
- if (TimeType.IDENTIFIER.equals(logicalId)) {
+ if (SqlTypes.TIME.getIdentifier().equals(logicalId)) {
return nullOr(
- value, Expressions.convert_(Expressions.call(value,
"getMillis"), int.class));
+ value, Expressions.divide(value,
Expressions.constant(NANOS_PER_MILLISECOND)));
} else if (SqlTypes.DATE.getIdentifier().equals(logicalId)) {
- value = nullOr(value, value);
+ return nullOr(value, value);
Review comment:
This is going to generate the code `value == null ? null : value`, which
does nothing. drop it all together?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
