You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/04/22 05:50:05 UTC

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1541: [MINOR] Add ability to specify time unit for TimestampBasedKeyGenerator

vinothchandar commented on a change in pull request #1541:
URL: https://github.com/apache/incubator-hudi/pull/1541#discussion_r412685370



##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/keygen/TimestampBasedKeyGenerator.java
##########
@@ -123,4 +132,8 @@ public HoodieKey getKey(GenericRecord record) {
       throw new HoodieDeltaStreamerException("Unable to parse input partition field :" + partitionVal, pe);
     }
   }
+
+  private long convertLongTimeToSeconds(Long partitionVal) {

Review comment:
       this is converting to ms, rather? 

##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/keygen/TimestampBasedKeyGenerator.java
##########
@@ -35,16 +35,21 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.TimeZone;
+import java.util.concurrent.TimeUnit;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
 /**
  * Key generator, that relies on timestamps for partitioning field. Still picks record key by name.
  */
 public class TimestampBasedKeyGenerator extends SimpleKeyGenerator {
 
   enum TimestampType implements Serializable {
-    UNIX_TIMESTAMP, DATE_STRING, MIXED, EPOCHMILLISECONDS

Review comment:
       this will break users setting EPOCHMILLISECONDS today.. Could we avoid this pain  and transparently handle this 

##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/keygen/TimestampBasedKeyGenerator.java
##########
@@ -35,16 +35,21 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.TimeZone;
+import java.util.concurrent.TimeUnit;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
 /**
  * Key generator, that relies on timestamps for partitioning field. Still picks record key by name.
  */
 public class TimestampBasedKeyGenerator extends SimpleKeyGenerator {
 
   enum TimestampType implements Serializable {
-    UNIX_TIMESTAMP, DATE_STRING, MIXED, EPOCHMILLISECONDS

Review comment:
       If this is set, we can make the timeunit be MILLISECONDS.. 




----------------------------------------------------------------
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:
users@infra.apache.org