wuchong commented on a change in pull request #13834:
URL: https://github.com/apache/flink/pull/13834#discussion_r519662950
##########
File path:
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java
##########
@@ -320,6 +339,34 @@ private void testField(
assertEquals(expectedCsv, new String(serializedRow));
}
+ private void testFieldForTime(
Review comment:
This is never used.
##########
File path:
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java
##########
@@ -150,6 +151,24 @@ public void testSerializeDeserializeCustomizedProperties()
throws Exception {
deserConfig,
";");
testField(STRING(), "null", "null", serConfig, deserConfig,
";"); // string because null literal has not been set
+ testField(TIME(3), "12:12:12.232",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.232342",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.232312",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2235",
LocalTime.parse("12:12:12.2") , deserConfig , ";");
+ testField(TIME(1), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12.45", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ int precision = 5;
+ String expectedMessage = String.format("Csv does not support
TIME type with precision: %d, it only supports precision 0 ~ 3.", precision);
+ try {
+ testField(TIME(0), "12:12:12.45",
LocalTime.parse("12:12:12") , deserConfig , ";");
Review comment:
This will not fail. Because it is not `TIME(5)`.
You should add `fail();` if you want to verify it should fail.
##########
File path:
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java
##########
@@ -150,6 +151,24 @@ public void testSerializeDeserializeCustomizedProperties()
throws Exception {
deserConfig,
";");
testField(STRING(), "null", "null", serConfig, deserConfig,
";"); // string because null literal has not been set
+ testField(TIME(3), "12:12:12.232",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.232342",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.232312",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2235",
LocalTime.parse("12:12:12.2") , deserConfig , ";");
+ testField(TIME(1), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12.45", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ int precision = 5;
+ String expectedMessage = String.format("Csv does not support
TIME type with precision: %d, it only supports precision 0 ~ 3.", precision);
Review comment:
Why not hard code the exception message?
##########
File path:
flink-formats/flink-csv/src/main/java/org/apache/flink/formats/csv/CsvToRowDataConverters.java
##########
@@ -221,12 +221,27 @@ private int convertToDate(JsonNode jsonNode) {
return (int)
Date.valueOf(jsonNode.asText()).toLocalDate().toEpochDay();
}
- private int convertToTime(JsonNode jsonNode) {
+ private CsvToRowDataConverter convertToTime(TimeType timeType) {
+ final int precision = timeType.getPrecision();
// csv currently is using Time.valueOf() to parse time string
- LocalTime localTime =
Time.valueOf(jsonNode.asText()).toLocalTime();
// TODO: FLINK-17525 support millisecond and nanosecond
// get number of milliseconds of the day
- return localTime.toSecondOfDay() * 1000;
+ if (precision > 3) {
+ throw new IllegalArgumentException("Csv does not
support TIME type " +
+ "with precision: " + precision + ", it only
supports precision 0 ~ 3.");
+ }
+ return jsonNode -> {
+ LocalTime localTime =
LocalTime.parse(jsonNode.asText());
+ int mills = (int) (localTime.toNanoOfDay() / 1000_000L);
+ if (precision == 2) {
+ mills = mills / 10 * 10;
Review comment:
Could you add a comment "this is for rounding off values out of
precision"?
##########
File path:
flink-formats/flink-csv/src/test/java/org/apache/flink/formats/csv/CsvRowDataSerDeSchemaTest.java
##########
@@ -150,6 +151,24 @@ public void testSerializeDeserializeCustomizedProperties()
throws Exception {
deserConfig,
";");
testField(STRING(), "null", "null", serConfig, deserConfig,
";"); // string because null literal has not been set
+ testField(TIME(3), "12:12:12.232",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.232342",
LocalTime.parse("12:12:12.232") , deserConfig , ";");
+ testField(TIME(3), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.23",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.232312",
LocalTime.parse("12:12:12.23") , deserConfig , ";");
+ testField(TIME(2), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2", LocalTime.parse("12:12:12.2")
, deserConfig , ";");
+ testField(TIME(1), "12:12:12.2235",
LocalTime.parse("12:12:12.2") , deserConfig , ";");
+ testField(TIME(1), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12", LocalTime.parse("12:12:12") ,
deserConfig , ";");
+ testField(TIME(0), "12:12:12.45", LocalTime.parse("12:12:12") ,
deserConfig , ";");
Review comment:
All of these only test the deserialization. We should also add tests for
serialization. I think we can use `testNullableField` for this purpose.
Besides, we can rename method `testField(fieldType, csvValue, value,
deserializationConfig, fieldDelimiter)` to `testFieldSerialization` to be more
specific.
----------------------------------------------------------------
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:
[email protected]
