ashish-kumar-sharma commented on a change in pull request #2550:
URL: https://github.com/apache/hive/pull/2550#discussion_r681062589
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFFromUnixTime.java
##########
@@ -60,7 +62,7 @@
private transient SimpleDateFormat formatter = new
SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
private transient String lastFormat = null;
-
+ private transient DateTimeFormatter FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
Review comment:
"yyyy" -> "uuuu"
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFFromUnixTime.java
##########
@@ -120,7 +98,7 @@ public void configure(MapredContext context) {
if (context != null) {
Review comment:
entire configure method is not required. You can remove the it.
##########
File path: ql/src/test/queries/clientpositive/udf5.q
##########
@@ -13,6 +13,8 @@ SELECT from_unixtime(unix_timestamp('2010-01-13 11:57:40',
'yyyy-MM-dd HH:mm:ss'
SELECT from_unixtime(unix_timestamp('2010-01-13 11:57:40', 'yyyy-MM-dd
HH:mm:ss'), 'MM/dd/yy HH:mm:ss'), from_unixtime(unix_timestamp('2010-01-13
11:57:40')) from dest1_n14;
+SELECT from_unixtime(unix_timestamp(cast('2010-01-13' as date)));
Review comment:
Also create a class TestGenericUDFFromUnixTime also add these test
cases. there as well. For reference -
https://github.com/apache/hive/blob/7b3ecf617a6d46f48a3b6f77e0339fd4ad95a420/ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFFromUtcTimestamp.java
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFFromUnixTime.java
##########
@@ -153,23 +127,14 @@ public Object evaluate(DeferredObject[] arguments) throws
HiveException {
unixtime = inputLongOI.get(arguments[0].get());
}
- Date date = new Date(unixtime * 1000L);
- result.set(formatter.format(date));
+ Instant i = Instant.ofEpochSecond(unixtime);
+ ZonedDateTime z = ZonedDateTime.ofInstant(i, timeZone);
+ result.set(z.format(FORMATTER));
return result;
}
- protected String getName() {
- return "from_unixtime";
- }
-
@Override
public String getDisplayString(String[] children) {
- StringBuilder sb = new StringBuilder(32);
- sb.append(getName());
- sb.append('(');
- sb.append(StringUtils.join(children, ", "));
- sb.append(')');
- return sb.toString();
+ return getStandardDisplayString(getFuncName(), children, ", ");
}
-
-}
+}
Review comment:
NIT: add empty at the end.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFFromUnixTime.java
##########
@@ -87,29 +75,19 @@ public ObjectInspector initialize(ObjectInspector[]
arguments) throws UDFArgumen
inputLongOI = (LongObjectInspector) arguments[0];
break;
default:
- throw new UDFArgumentException("The function " +
getName().toUpperCase()
+ throw new UDFArgumentException("The function " +
getFuncName().toUpperCase()
+ " takes only int/long types for first argument. Got Type:" +
arg0OI.getPrimitiveCategory().name());
}
if (arguments.length == 2) {
- PrimitiveObjectInspector arg1OI = (PrimitiveObjectInspector)
arguments[1];
- switch (arg1OI.getPrimitiveCategory()) {
- case CHAR:
- case VARCHAR:
- case STRING:
- inputTextConverter = ObjectInspectorConverters.getConverter(arg1OI,
- PrimitiveObjectInspectorFactory.javaStringObjectInspector);
- break;
- default:
- throw new UDFArgumentException("The function " +
getName().toUpperCase()
- + " takes only string type for second argument. Got Type:" +
arg1OI.getPrimitiveCategory().name());
- }
+ checkArgGroups(arguments, 1, inputTypes, STRING_GROUP);
+ obtainStringConverter(arguments, 1, inputTypes, converters);
}
if (timeZone == null) {
Review comment:
remove timeZone == null check. Always override the timeZone variable
with value stored in session or default value.
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFFromUnixTime.java
##########
@@ -153,23 +127,14 @@ public Object evaluate(DeferredObject[] arguments) throws
HiveException {
unixtime = inputLongOI.get(arguments[0].get());
}
- Date date = new Date(unixtime * 1000L);
- result.set(formatter.format(date));
+ Instant i = Instant.ofEpochSecond(unixtime);
Review comment:
int is used for seconds and long is used for milliseconds. Put a check
for the same and call Instant.ofEpochSecond() and Instant.ofEpochMili()
respectively
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
