SourabhBadhya commented on code in PR #5779:
URL: https://github.com/apache/hive/pull/5779#discussion_r2094351544
##########
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestAvroSerializer.java:
##########
@@ -125,7 +125,7 @@ public void canSerializeDoubles() throws SerDeException,
IOException {
public void canSerializeTimestamps() throws SerDeException, IOException {
singleFieldTest("timestamp1", Timestamp.valueOf("2011-01-01
00:00:00").toEpochMilli(),
"\"" + AvroSerDe.AVRO_LONG_TYPE_NAME + "\"," +
- "\"logicalType\":\"" + AvroSerDe.TIMESTAMP_TYPE_NAME + "\"");
+ "\"logicalType\":\"" + AvroSerDe.TIMESTAMP_TYPE_NAME_MILLIS + "\"");
Review Comment:
Remove this and add explicit test cases for micros.
##########
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestAvroObjectInspectorGenerator.java:
##########
@@ -227,7 +227,7 @@ public class TestAvroObjectInspectorGenerator {
" \"fields\" : [\n" +
" {\"name\":\"timestampField\", " +
" \"type\":\"" + AvroSerDe.AVRO_LONG_TYPE_NAME + "\", " +
- " \"logicalType\":\"" + AvroSerDe.TIMESTAMP_TYPE_NAME + "\"}" +
+ " \"logicalType\":\"" + AvroSerDe.TIMESTAMP_TYPE_NAME_MILLIS +
"\"}" +
Review Comment:
Remove this and add explicit test cases for micros.
##########
ql/src/test/queries/clientpositive/avro_hybrid_mixed_timestamp.q:
##########
@@ -5,13 +5,13 @@ stored as avro;
INSERT INTO hybrid_table VALUES
('2012-02-21 07:08:09.123'),
-('2014-02-11 07:08:09.123'),
+('2014-02-11 07:08:09.123456'),
('1947-02-11 07:08:09.123'),
-('8200-02-11 07:08:09.123'),
-('1012-02-21 07:15:11.123'),
-('1014-02-11 07:15:11.123'),
+('8200-02-11 07:08:09.123456'),
+('1012-02-21 07:15:11.12345'),
+('1014-02-11 07:15:11.1234'),
('0947-02-11 07:15:11.123'),
-('0200-02-11 07:15:11.123');
+('0200-02-11 07:15:11.1234');
Review Comment:
Always refrain from modifying existing tests. Considering adding new tests.
##########
ql/src/test/queries/clientpositive/avro_timestamp_micros.q:
##########
@@ -0,0 +1,9 @@
+CREATE EXTERNAL TABLE micros_table(`dt` timestamp)
+STORED AS AVRO;
+
+INSERT INTO micros_table VALUES
+(cast('2024-08-09 14:08:26.326107' as timestamp)),
+('2012-02-21 07:08:09.123'),
+('1014-02-11 07:15:11.12345');
+
+SELECT * FROM micros_table;
Review Comment:
Consider adding a test with timestamp-micros as a column and
timestamp-millis as a column.
##########
serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerializer.java:
##########
@@ -231,6 +232,14 @@ private Object serializePrimitive(TypeInfo typeInfo,
PrimitiveObjectInspector fi
case TIMESTAMP:
Timestamp timestamp =
((TimestampObjectInspector)
fieldOI).getPrimitiveJavaObject(structFieldData);
+ LogicalType logicalType = schema.getLogicalType();
+ if (logicalType != null &&
logicalType.getName().equalsIgnoreCase(AvroSerDe.TIMESTAMP_TYPE_NAME_MICROS)) {
+ long micros = defaultProleptic ? timestamp.toEpochMicro() :
+
CalendarUtils.convertTimeToHybridMicros(timestamp.toEpochMicro());
+ timestamp = TimestampTZUtil.convertTimestampToZone(
+ Timestamp.ofEpochMicro(micros),
TimeZone.getDefault().toZoneId(), ZoneOffset.UTC, legacyConversion);
+ return timestamp.toEpochMicro();
+ }
long millis = defaultProleptic ? timestamp.toEpochMilli() :
CalendarUtils.convertTimeToHybrid(timestamp.toEpochMilli());
timestamp = TimestampTZUtil.convertTimestampToZone(
Review Comment:
Consider creating a function which would pass the required fields and return
timestamp object.
This is because of enhancing extensibility for `timestamp-nanos`.
##########
ql/src/test/queries/clientpositive/avro_proleptic_mixed_timestamp.q:
##########
@@ -7,13 +7,13 @@ stored as avro;
INSERT INTO hybrid_table VALUES
('2012-02-21 07:08:09.123'),
-('2014-02-11 07:08:09.123'),
-('1947-02-11 07:08:09.123'),
+('2014-02-11 07:08:09.123456'),
+('1947-02-11 07:08:09.1234'),
('8200-02-11 07:08:09.123'),
-('1012-02-21 07:15:11.123'),
+('1012-02-21 07:15:11.12345'),
('1014-02-11 07:15:11.123'),
-('0947-02-11 07:15:11.123'),
-('0200-02-11 07:15:11.123');
+('0947-02-11 07:15:11.12345'),
+('0200-02-11 07:15:11.123456');
Review Comment:
Refrain from changing existing tests. Add a new test for timestamp-micros
explicitly mentioning its column type.
##########
serde/src/java/org/apache/hadoop/hive/serde2/avro/SchemaToTypeInfo.java:
##########
@@ -197,6 +196,25 @@ private static int getIntValue(Object obj) {
return value;
}
+ /**
+ * Checks if the given Avro schema represents a timestamp type
+ * based on its logical type property.
+ */
+ public static boolean isTimestampType(Schema schema) {
+ if (schema == null) {
+ return false;
+ }
+
+ String logicalType = schema.getProp(AvroSerDe.AVRO_PROP_LOGICAL_TYPE);
+ if (logicalType == null) {
+ return false;
+ }
+
+ // Supported timestamp logical types (extend this set as needed)
+ return logicalType.equalsIgnoreCase(AvroSerDe.TIMESTAMP_TYPE_NAME_MILLIS)
||
+ logicalType.equalsIgnoreCase(AvroSerDe.TIMESTAMP_TYPE_NAME_MICROS);
Review Comment:
Its better to replace this code with -
`AvroSerDe.TIMESTAMP_TYPE_NAME_MILLIS.equalsIgnoreCase(logicalType) ||
AvroSerDe.TIMESTAMP_TYPE_NAME_MICROS.equalsIgnoreCase(logicalType);`
##########
serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java:
##########
@@ -261,7 +261,7 @@ public void createAvroDateSchema() {
public void createAvroTimestampSchema() {
final String specificSchema = "{" +
"\"type\":\"long\"," +
- "\"logicalType\":\"timestamp-millis\"}";
+ "\"logicalType\":\"timestamp-micros\"}";
Review Comment:
Remove this and add explicit test cases for micros.
--
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.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
