Skip to content

Commit

Permalink
Remove support for illogical types in rfc2822 record decoder
Browse files Browse the repository at this point in the history
  • Loading branch information
@charlesjmorgan @losipiuk
charlesjmorgan authored and losipiuk committed Sep 14, 2020
1 parent 456e1fa commit 3ef8cfd
Show file tree
Hide file tree
Showing 8 changed files with 15 additions and 71 deletions.
13 changes: 9 additions & 4 deletions presto-docs/src/main/sphinx/connector/kafka.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -821,12 +821,17 @@ which can be specified via ``dataFormat`` attribute.
| | ``VARCHAR`` | |
| | ``VARCHAR(x)`` | |
+-------------------------------------+--------------------------------------------------------------------------------+
| | ``DATE`` | ``custom-date-time``, ``iso8601`` |
+-------------------------------------+--------------------------------------------------------------------------------+
| | ``TIME`` | ``custom-date-time``, ``iso8601``, ``milliseconds-since-epoch``, |
| | | ``seconds-since-epoch`` |
+-------------------------------------+--------------------------------------------------------------------------------+
| | ``TIME WITH TIME ZONE`` | ``custom-date-time``, ``iso8601`` |
+-------------------------------------+--------------------------------------------------------------------------------+
| | ``TIMESTAMP`` | ``custom-date-time``, ``iso8601``, ``rfc2822``, |
| | ``TIMESTAMP WITH TIME ZONE`` | ``milliseconds-since-epoch``, ``seconds-since-epoch`` |
| | ``TIME`` | |
| | ``TIME WITH TIME ZONE`` | |
| | | ``milliseconds-since-epoch``, ``seconds-since-epoch`` |
+-------------------------------------+--------------------------------------------------------------------------------+
| ``DATE`` | ``custom-date-time``, ``iso8601``, ``rfc2822``, |
| | ``TIMESTAMP WITH TIME ZONE`` | ``custom-date-time``, ``iso8601``, ``rfc2822`` |
+-------------------------------------+--------------------------------------------------------------------------------+


Expand Down
9 changes: 0 additions & 9 deletions presto-kafka/src/test/java/io/prestosql/plugin/kafka/TestKafkaIntegrationSmokeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,10 @@ public void testReadAllDataTypes()
"\"j_timestamp_rfc2822\" : \"Fri Feb 09 13:15:19 Z 2018\" ," +
"\"j_timestamp_custom\" : \"02/2018/09 13:15:20\" ," +
"\"j_date_iso8601\" : \"2018-02-11\" ," +
"\"j_date_rfc2822\" : \"Mon Feb 12 13:15:16 Z 2018\" ," +
"\"j_date_custom\" : \"2018/13/02\" ," +
"\"j_time_milliseconds_since_epoch\" : \"47716000\" ," +
"\"j_time_seconds_since_epoch\" : \"47717\" ," +
"\"j_time_iso8601\" : \"13:15:18\" ," +
"\"j_time_rfc2822\" : \"Thu Jan 01 13:15:19 Z 1970\" ," +
"\"j_time_custom\" : \"15:13:20\" ," +
"\"j_timestamptz_milliseconds_since_epoch\" : \"1518182116000\" ," +
"\"j_timestamptz_seconds_since_epoch\" : \"1518182117\" ," +
Expand All @@ -207,7 +205,6 @@ public void testReadAllDataTypes()
"\"j_timetz_milliseconds_since_epoch\" : \"47716000\" ," +
"\"j_timetz_seconds_since_epoch\" : \"47717\" ," +
"\"j_timetz_iso8601\" : \"13:15:18+00:00\" ," +
"\"j_timetz_rfc2822\" : \"Thu Jan 01 13:15:19 Z 1970\" ," +
"\"j_timetz_custom\" : \"15:13:20\" }";

insertData("read_test.all_datatypes_json", json.getBytes(UTF_8));
Expand All @@ -226,12 +223,10 @@ public void testReadAllDataTypes()
", c_timestamp_rfc2822 " +
", c_timestamp_custom " +
", c_date_iso8601 " +
", c_date_rfc2822 " +
", c_date_custom " +
", c_time_milliseconds_since_epoch " +
", c_time_seconds_since_epoch " +
", c_time_iso8601 " +
", c_time_rfc2822 " +
", c_time_custom " +
// H2 does not support TIMESTAMP WITH TIME ZONE so cast to VARCHAR
", cast(c_timestamptz_milliseconds_since_epoch as VARCHAR) " +
Expand All @@ -243,7 +238,6 @@ public void testReadAllDataTypes()
", cast(c_timetz_milliseconds_since_epoch as VARCHAR) " +
", cast(c_timetz_seconds_since_epoch as VARCHAR) " +
", cast(c_timetz_iso8601 as VARCHAR) " +
", cast(c_timetz_rfc2822 as VARCHAR) " +
", cast(c_timetz_custom as VARCHAR) " +
"FROM read_test.all_datatypes_json ",
"VALUES (" +
Expand All @@ -260,12 +254,10 @@ public void testReadAllDataTypes()
", TIMESTAMP '2018-02-09 13:15:19'" +
", TIMESTAMP '2018-02-09 13:15:20'" +
", DATE '2018-02-11'" +
", DATE '2018-02-12'" +
", DATE '2018-02-13'" +
", TIME '13:15:16'" +
", TIME '13:15:17'" +
", TIME '13:15:18'" +
", TIME '13:15:19'" +
", TIME '13:15:20'" +
", '2018-02-09 13:15:16.000 UTC'" +
", '2018-02-09 13:15:17.000 UTC'" +
Expand All @@ -275,7 +267,6 @@ public void testReadAllDataTypes()
", '13:15:16.000+00:00'" +
", '13:15:17.000+00:00'" +
", '13:15:18.000+00:00'" +
", '13:15:19.000+00:00'" +
", '13:15:20.000+00:00'" +
")");
}
Expand Down
18 changes: 0 additions & 18 deletions presto-kafka/src/test/resources/read_test/all_datatypes_json.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,6 @@
"mapping": "j_date_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_date_rfc2822",
"type": "DATE",
"mapping": "j_date_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_date_custom",
"type": "DATE",
Expand All @@ -108,12 +102,6 @@
"mapping": "j_time_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_time_rfc2822",
"type": "TIME",
"mapping": "j_time_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_time_custom",
"type": "TIME",
Expand Down Expand Up @@ -170,12 +158,6 @@
"mapping": "j_timetz_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_timetz_rfc2822",
"type": "TIME WITH TIME ZONE",
"mapping": "j_timetz_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_timetz_custom",
"type": "TIME WITH TIME ZONE",
Expand Down
18 changes: 0 additions & 18 deletions ...sources/docker/presto-product-tests/conf/presto/etc/catalog/kafka/all_datatypes_json.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,6 @@
"mapping": "j_date_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_date_rfc2822",
"type": "DATE",
"mapping": "j_date_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_date_custom",
"type": "DATE",
Expand All @@ -108,12 +102,6 @@
"mapping": "j_time_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_time_rfc2822",
"type": "TIME",
"mapping": "j_time_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_time_custom",
"type": "TIME",
Expand Down Expand Up @@ -170,12 +158,6 @@
"mapping": "j_timetz_iso8601",
"dataFormat": "iso8601"
},
{
"name": "c_timetz_rfc2822",
"type": "TIME WITH TIME ZONE",
"mapping": "j_timetz_rfc2822",
"dataFormat": "rfc2822"
},
{
"name": "c_timetz_custom",
"type": "TIME WITH TIME ZONE",
Expand Down
9 changes: 0 additions & 9 deletions presto-product-tests/src/main/java/io/prestosql/tests/kafka/TestKafkaSmokeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,10 @@ public Requirement getRequirements(Configuration configuration)
"\"j_timestamp_rfc2822\" : \"Fri Feb 09 13:15:19 Z 2018\" ," +
"\"j_timestamp_custom\" : \"02/2018/09 13:15:20\" ," +
"\"j_date_iso8601\" : \"2018-02-11\" ," +
"\"j_date_rfc2822\" : \"Mon Feb 12 13:15:16 Z 2018\" ," +
"\"j_date_custom\" : \"2018/13/02\" ," +
"\"j_time_milliseconds_since_epoch\" : \"47716000\" ," +
"\"j_time_seconds_since_epoch\" : \"47717\" ," +
"\"j_time_iso8601\" : \"13:15:18\" ," +
"\"j_time_rfc2822\" : \"Thu Jan 01 13:15:19 Z 1970\" ," +
"\"j_time_custom\" : \"15:13:20\" ," +
"\"j_timestamptz_milliseconds_since_epoch\" : \"1518182116000\" ," +
"\"j_timestamptz_seconds_since_epoch\" : \"1518182117\" ," +
Expand All @@ -268,7 +266,6 @@ public Requirement getRequirements(Configuration configuration)
"\"j_timetz_milliseconds_since_epoch\" : \"47716000\" ," +
"\"j_timetz_seconds_since_epoch\" : \"47717\" ," +
"\"j_timetz_iso8601\" : \"13:15:18Z\" ," +
"\"j_timetz_rfc2822\" : \"Thu Jan 01 13:15:19 Z 1970\" ," +
"\"j_timetz_custom\" : \"15:13:20\" }"))),
1,
1));
Expand Down Expand Up @@ -298,12 +295,10 @@ public void testSelectAllJsonTable()
row("c_timestamp_rfc2822", "timestamp(3)"),
row("c_timestamp_custom", "timestamp(3)"),
row("c_date_iso8601", "date"),
row("c_date_rfc2822", "date"),
row("c_date_custom", "date"),
row("c_time_milliseconds_since_epoch", "time(3)"),
row("c_time_seconds_since_epoch", "time(3)"),
row("c_time_iso8601", "time(3)"),
row("c_time_rfc2822", "time(3)"),
row("c_time_custom", "time(3)"),
row("c_timestamptz_milliseconds_since_epoch", "timestamp(3) with time zone"),
row("c_timestamptz_seconds_since_epoch", "timestamp(3) with time zone"),
Expand All @@ -313,7 +308,6 @@ public void testSelectAllJsonTable()
row("c_timetz_milliseconds_since_epoch", "time(3) with time zone"),
row("c_timetz_seconds_since_epoch", "time(3) with time zone"),
row("c_timetz_iso8601", "time(3) with time zone"),
row("c_timetz_rfc2822", "time(3) with time zone"),
row("c_timetz_custom", "time(3) with time zone"));

assertThat(query(format("select * from %s.%s.%s", KAFKA_CATALOG, SCHEMA_NAME, ALL_DATATYPES_JSON_TABLE_NAME))).containsOnly(row(
Expand All @@ -330,12 +324,10 @@ public void testSelectAllJsonTable()
Timestamp.valueOf(LocalDateTime.of(2018, 2, 9, 13, 15, 19)),
Timestamp.valueOf(LocalDateTime.of(2018, 2, 9, 13, 15, 20)),
Date.valueOf(LocalDate.of(2018, 2, 11)),
Date.valueOf(LocalDate.of(2018, 2, 12)),
Date.valueOf(LocalDate.of(2018, 2, 13)),
Time.valueOf(LocalTime.of(13, 15, 16)),
Time.valueOf(LocalTime.of(13, 15, 17)),
Time.valueOf(LocalTime.of(13, 15, 18)),
Time.valueOf(LocalTime.of(13, 15, 19)),
Time.valueOf(LocalTime.of(13, 15, 20)),
// different because product test framework converts to java.sql.Timestamp
Timestamp.valueOf(LocalDateTime.of(2018, 2, 9, 19, 0, 16)),
Expand All @@ -346,7 +338,6 @@ public void testSelectAllJsonTable()
Time.valueOf(LocalTime.of(18, 45, 16)),
Time.valueOf(LocalTime.of(18, 45, 17)),
Time.valueOf(LocalTime.of(18, 45, 18)),
Time.valueOf(LocalTime.of(18, 45, 19)),
Time.valueOf(LocalTime.of(18, 45, 20))));
}

Expand Down
5 changes: 1 addition & 4 deletions presto-record-decoder/src/main/java/io/prestosql/decoder/json/RFC2822JsonFieldDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@

import static io.prestosql.decoder.DecoderErrorCode.DECODER_CONVERSION_NOT_SUPPORTED;
import static io.prestosql.decoder.json.JsonRowDecoderFactory.throwUnsupportedColumnType;
import static io.prestosql.spi.type.DateType.DATE;
import static io.prestosql.spi.type.TimeType.TIME;
import static io.prestosql.spi.type.TimeWithTimeZoneType.TIME_WITH_TIME_ZONE;
import static io.prestosql.spi.type.TimeZoneKey.getTimeZoneKey;
import static io.prestosql.spi.type.TimestampType.TIMESTAMP_MILLIS;
import static io.prestosql.spi.type.TimestampWithTimeZoneType.TIMESTAMP_WITH_TIME_ZONE;
Expand All @@ -46,7 +43,7 @@
public class RFC2822JsonFieldDecoder
implements JsonFieldDecoder
{
private static final Set<Type> SUPPORTED_TYPES = ImmutableSet.of(DATE, TIME, TIME_WITH_TIME_ZONE, TIMESTAMP_MILLIS, TIMESTAMP_WITH_TIME_ZONE);
private static final Set<Type> SUPPORTED_TYPES = ImmutableSet.of(TIMESTAMP_MILLIS, TIMESTAMP_WITH_TIME_ZONE);

/**
* Todo - configurable time zones and locales.
Expand Down
5 changes: 4 additions & 1 deletion presto-record-decoder/src/test/java/io/prestosql/decoder/json/TestJsonDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,10 @@ public void testSupportedDataTypeValidation()
singleColumnDecoder(createUnboundedVarcharType(), null);
singleColumnDecoder(createVarcharType(100), null);

for (String dataFormat : ImmutableSet.of("iso8601", "custom-date-time", "rfc2822")) {
singleColumnDecoder(TIMESTAMP_MILLIS, "rfc2822");
singleColumnDecoder(TIMESTAMP_WITH_TIME_ZONE, "rfc2822");

for (String dataFormat : ImmutableSet.of("iso8601", "custom-date-time")) {
singleColumnDecoder(DATE, dataFormat);
singleColumnDecoder(TIME, dataFormat);
singleColumnDecoder(TIME_WITH_TIME_ZONE, dataFormat);
Expand Down
9 changes: 1 addition & 8 deletions ...o-record-decoder/src/test/java/io/prestosql/decoder/json/TestRFC2822JsonFieldDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@
import org.testng.annotations.Test;

import static io.prestosql.spi.type.DateTimeEncoding.packDateTimeWithZone;
import static io.prestosql.spi.type.DateTimeEncoding.packTimeWithTimeZone;
import static io.prestosql.spi.type.DateType.DATE;
import static io.prestosql.spi.type.TimeType.TIME;
import static io.prestosql.spi.type.TimeWithTimeZoneType.TIME_WITH_TIME_ZONE;
import static io.prestosql.spi.type.TimeZoneKey.UTC_KEY;
import static io.prestosql.spi.type.TimeZoneKey.getTimeZoneKeyForOffset;
import static io.prestosql.spi.type.TimestampType.TIMESTAMP_MILLIS;
Expand All @@ -34,9 +30,6 @@ public class TestRFC2822JsonFieldDecoder
@Test
public void testDecode()
{
tester.assertDecodedAs("\"Mon Feb 12 13:15:16 Z 2018\"", DATE, 17574); // TODO should it be supported really?
tester.assertDecodedAs("\"Thu Jan 01 13:15:19 Z 1970\"", TIME, 47_719_000_000_000_000L); // TODO should it be supported really?
tester.assertDecodedAs("\"Thu Jan 01 13:15:19 Z 1970\"", TIME_WITH_TIME_ZONE, packTimeWithTimeZone(47_719_000_000_000L, 0)); // TODO should it be supported really?
tester.assertDecodedAs("\"Fri Feb 09 13:15:19 Z 2018\"", TIMESTAMP_MILLIS, 1_518_182_119_000_000L);
tester.assertDecodedAs("\"Fri Feb 09 13:15:19 Z 2018\"", TIMESTAMP_WITH_TIME_ZONE, packDateTimeWithZone(1518182119000L, UTC_KEY));
tester.assertDecodedAs("\"Fri Feb 09 15:15:19 +02:00 2018\"", TIMESTAMP_MILLIS, 1_518_182_119_000_000L);
Expand All @@ -46,7 +39,7 @@ public void testDecode()
@Test
public void testDecodeNulls()
{
for (Type type : asList(DATE, TIME, TIME_WITH_TIME_ZONE, TIMESTAMP_MILLIS, TIMESTAMP_WITH_TIME_ZONE)) {
for (Type type : asList(TIMESTAMP_MILLIS, TIMESTAMP_WITH_TIME_ZONE)) {
tester.assertDecodedAsNull("null", type);
tester.assertMissingDecodedAsNull(type);
}
Expand Down

0 comments on commit 3ef8cfd

Please sign in to comment.