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/08/05 11:26:23 UTC

[GitHub] [hudi] Mathieu1124 opened a new pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Mathieu1124 opened a new pull request #1920:
URL: https://github.com/apache/hudi/pull/1920


   …n using TimestampBasedKeyGenerator
   
   ## *Tips*
   - *Thank you very much for contributing to Apache Hudi.*
   - *Please review https://hudi.apache.org/contributing.html before opening a pull request.*
   
   ## What is the purpose of the pull request
   
   *Fix unable to parse input partition field :1 exception when using TimestampBasedKeyGenerator *
   
   ## Brief change log
   
   scene to reproduce：
   
       1. use TimestampBasedKeyGenerator
       2. set hoodie.deltastreamer.keygen.timebased.timestamp.type = DATE_STRING
       3. partitionpath field value is null
   
   when partitionpath field value is null TimestampBasedKeyGenerator will set it to1L, which can not be parsed correctly.
   `User class threw exception: java.util.concurrent.ExecutionException: org.apache.hudi.exception.HoodieException: Job aborted due to stage failure: Task 0 in stage 1.0 failed 4 times, most recent failure: Lost task 0.3 in stage 1.0 (TID 4, prod-t3-data-lake-007, executor 6): org.apache.hudi.exception.HoodieDeltaStreamerException: Unable to parse input partition field :1
    at org.apache.hudi.keygen.TimestampBasedKeyGenerator.getPartitionPath(TimestampBasedKeyGenerator.java:156)
    at org.apache.hudi.keygen.CustomKeyGenerator.getPartitionPath(CustomKeyGenerator.java:108)
    at org.apache.hudi.keygen.CustomKeyGenerator.getKey(CustomKeyGenerator.java:78)
    at org.apache.hudi.utilities.deltastreamer.DeltaSync.lambda$readFromSource$9fce03f0$1(DeltaSync.java:343)
    at org.apache.spark.api.java.JavaPairRDD$$anonfun$toScalaFunction$1.apply(JavaPairRDD.scala:1040)
    at scala.collection.Iterator$$anon$11.next(Iterator.scala:410)
    at scala.collection.Iterator$$anon$10.next(Iterator.scala:394)
    at scala.collection.Iterator$class.foreach(Iterator.scala:891)
    at scala.collection.AbstractIterator.foreach(Iterator.scala:1334)
    at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:59)
    at scala.collection.mutable.ArrayBuffer.$plus$plus$eq(ArrayBuffer.scala:104)
    at scala.collection.mutable.ArrayBuffer.$plus$plus$eq(ArrayBuffer.scala:48)
    at scala.collection.TraversableOnce$class.to(TraversableOnce.scala:310)
    at scala.collection.AbstractIterator.to(Iterator.scala:1334)
    at scala.collection.TraversableOnce$class.toBuffer(TraversableOnce.scala:302)
    at scala.collection.AbstractIterator.toBuffer(Iterator.scala:1334)
    at scala.collection.TraversableOnce$class.toArray(TraversableOnce.scala:289)
    at scala.collection.AbstractIterator.toArray(Iterator.scala:1334)
    at org.apache.spark.rdd.RDD$$anonfun$take$1$$anonfun$29.apply(RDD.scala:1364)
    at org.apache.spark.rdd.RDD$$anonfun$take$1$$anonfun$29.apply(RDD.scala:1364)
    at org.apache.spark.SparkContext$$anonfun$runJob$5.apply(SparkContext.scala:2121)
    at org.apache.spark.SparkContext$$anonfun$runJob$5.apply(SparkContext.scala:2121)
    at org.apache.spark.scheduler.ResultTask.runTask(ResultTask.scala:90)
    at org.apache.spark.scheduler.Task.run(Task.scala:121)
    at org.apache.spark.executor.Executor$TaskRunner$$anonfun$11.apply(Executor.scala:407)
    at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1408)
    at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:413)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)
    Caused by: java.lang.RuntimeException: hoodie.deltastreamer.keygen.timebased.timestamp.scalar.time.unit is not specified but scalar it supplied as time value
    at org.apache.hudi.keygen.TimestampBasedKeyGenerator.convertLongTimeToMillis(TimestampBasedKeyGenerator.java:163)
    at org.apache.hudi.keygen.TimestampBasedKeyGenerator.getPartitionPath(TimestampBasedKeyGenerator.java:138)
    ... 29 more`
   
   
   ## Verify this pull request
   
   This pull request is already covered by org.apache.hudi.keygen.TestTimestampBasedKeyGenerator#testTimestampBasedKeyGenerator*.
   
   ## Committer checklist
   
    - [ ] Has a corresponding JIRA in PR title & commit
    
    - [ ] Commit message is descriptive of the change
    
    - [ ] CI is green
   
    - [ ] Necessary doc changes done or have another open PR
          
    - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.


----------------------------------------------------------------
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

[GitHub] [hudi] Mathieu1124 commented on pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
Mathieu1124 commented on pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#issuecomment-669142868


   @yanghua @pratyakshsharma @afilipchik please take a look when free 


----------------------------------------------------------------
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

[GitHub] [hudi] Mathieu1124 commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
Mathieu1124 commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r465826414



##########
File path: hudi-spark/src/test/java/org/apache/hudi/keygen/TestTimestampBasedKeyGenerator.java
##########
@@ -107,6 +107,13 @@ public void testTimestampBasedKeyGenerator() throws IOException {
     properties = getBaseKeyConfig("DATE_STRING", "yyyy-MM-dd hh", "GMT", null);
     HoodieKey hk4 = new TimestampBasedKeyGenerator(properties).getKey(baseRecord);
     assertEquals("2020-01-06 12", hk4.getPartitionPath());
+
+    // timestamp is DATE_STRING, timezone is GMT+8:00, createTime is null
+    baseRecord.put("createTime", null);
+    properties = getBaseKeyConfig("DATE_STRING", "yyyy-MM-dd hh:mm:ss", "GMT+8:00", null);

Review comment:
       > Let us add a test case for the scenario when timestampType is SCALAR or EPOCH_MILLISECONDS and the value of the field is null?
   
   done




----------------------------------------------------------------
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

[GitHub] [hudi] wangxianghu commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
wangxianghu commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r474739944



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java
##########
@@ -199,6 +202,32 @@ private String getPartitionPath(Object partitionVal) {
     return hiveStylePartitioning ? getPartitionPathFields().get(0) + "=" + partitionPath : partitionPath;
   }
 
+  /**
+   * Set default value to partitionVal if the input value of partitionPathField is null.
+   */
+  private Object getDefaultPartitionVal() {
+    Object result = 1L;
+    if (timestampType == TimestampType.DATE_STRING || timestampType == TimestampType.MIXED) {
+      // since partitionVal is null, we can set a default value of any format as TIMESTAMP_INPUT_DATE_FORMAT_PROP
+      // configured, here we take the first.
+      String delimiter = ((HoodieDateTimeParserImpl) parser).getConfigInputDateFormatDelimiter();

Review comment:
       > For safety, you should judge if the parser is the instance of `HoodieDateTimeParserImpl `. Although, there is only one implementation currently.
   
   done, I have refactored HoodieDateTimeParser to expose some tool methods.




----------------------------------------------------------------
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

[GitHub] [hudi] yanghua commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r474642483



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java
##########
@@ -199,6 +202,32 @@ private String getPartitionPath(Object partitionVal) {
     return hiveStylePartitioning ? getPartitionPathFields().get(0) + "=" + partitionPath : partitionPath;
   }
 
+  /**
+   * Set default value to partitionVal if the input value of partitionPathField is null.
+   */
+  private Object getDefaultPartitionVal() {
+    Object result = 1L;
+    if (timestampType == TimestampType.DATE_STRING || timestampType == TimestampType.MIXED) {
+      // since partitionVal is null, we can set a default value of any format as TIMESTAMP_INPUT_DATE_FORMAT_PROP
+      // configured, here we take the first.
+      String delimiter = ((HoodieDateTimeParserImpl) parser).getConfigInputDateFormatDelimiter();

Review comment:
       For safety, you should judge if the parser is the instance of `HoodieDateTimeParserImpl `. Although, there is only one implementation currently.




----------------------------------------------------------------
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

[GitHub] [hudi] pratyakshsharma commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
pratyakshsharma commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r465764498



##########
File path: hudi-spark/src/test/java/org/apache/hudi/keygen/TestTimestampBasedKeyGenerator.java
##########
@@ -107,6 +107,13 @@ public void testTimestampBasedKeyGenerator() throws IOException {
     properties = getBaseKeyConfig("DATE_STRING", "yyyy-MM-dd hh", "GMT", null);
     HoodieKey hk4 = new TimestampBasedKeyGenerator(properties).getKey(baseRecord);
     assertEquals("2020-01-06 12", hk4.getPartitionPath());
+
+    // timestamp is DATE_STRING, timezone is GMT+8:00, createTime is null
+    baseRecord.put("createTime", null);
+    properties = getBaseKeyConfig("DATE_STRING", "yyyy-MM-dd hh:mm:ss", "GMT+8:00", null);

Review comment:
       Let us add a test case for the scenario when timestampType is SCALAR or EPOCH_MILLISECONDS and the value of the field is null?




----------------------------------------------------------------
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

[GitHub] [hudi] yanghua merged pull request #1920: [HUDI-1150] Fix unable to parse input partition field :1 exception when using TimestampBasedKeyGenerator

Posted by GitBox <gi...@apache.org>.
yanghua merged pull request #1920:
URL: https://github.com/apache/hudi/pull/1920


   


----------------------------------------------------------------
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

[GitHub] [hudi] wangxianghu commented on pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
wangxianghu commented on pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#issuecomment-678028232


   > @wangxianghu conflicts should be fixed.
   
   @yanghua This pr is ready for review now


----------------------------------------------------------------
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

[GitHub] [hudi] wangxianghu commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
wangxianghu commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r475100373



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/parser/AbstractHoodieDateTimeParser.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.keygen.parser;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.keygen.TimestampBasedKeyGenerator;
+import org.joda.time.DateTimeZone;
+import org.joda.time.format.DateTimeFormatter;
+
+import java.io.Serializable;
+import java.util.TimeZone;
+
+public abstract class AbstractHoodieDateTimeParser implements Serializable {

Review comment:
       > I am not sure if it's a good design. You make a detailed implementation pull-up into an abstraction. Why? `getInputDateTimeZone` and `getOutputDateTimeZone ` should be abstract method and be pushed down to a sub-class like before. Moreover, you can see it contains if/else block, it is full of changes.
   > 
   > The main benefit of abstract classes is to provide templates for the implementation of some subclasses. Instead of thinking that the default implementation is a template and marking it as an abstract class.
   > 
   > So I think the original design is good. WDYT?
   
   @yanghua thanks for your review. Make it an abstract class can allow us to reuse some tool methods, such as `getConfigInputDateFormatDelimiter()`.
    The `getInputDateTimeZone` and `getOutputDateTimeZone` has been pushed down to the sub-class now, it is better to be extendable.
   please take another look when free, thanks.




----------------------------------------------------------------
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

[GitHub] [hudi] yanghua commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r475094341



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/parser/AbstractHoodieDateTimeParser.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.keygen.parser;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.keygen.TimestampBasedKeyGenerator;
+import org.joda.time.DateTimeZone;
+import org.joda.time.format.DateTimeFormatter;
+
+import java.io.Serializable;
+import java.util.TimeZone;
+
+public abstract class AbstractHoodieDateTimeParser implements Serializable {

Review comment:
       I am not sure if it's a good design. You make a detailed implementation pull-up into an abstraction. Why? `getInputDateTimeZone` and `getOutputDateTimeZone ` should be abstract method and be pushed down to a sub-class like before. Moreover, you can see it contains if/else block, it is full of changes.
   
   The main benefit of abstract classes is to provide templates for the implementation of some subclasses. Instead of thinking that the default implementation is a template and marking it as an abstract class.
   
   So I think the original design is good. WDYT?




----------------------------------------------------------------
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

[GitHub] [hudi] wangxianghu commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
wangxianghu commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r471356023



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java
##########
@@ -170,6 +173,32 @@ public String getPartitionPath(GenericRecord record) {
     }
   }
 
+  /**
+   * Set default value to partitionVal if the input value of partitionPathField is null.
+   */
+  private Object getDefaultPartitionVal() {
+    Object result = 1L;

Review comment:
       > If you define a variable named `result`, then stop using `return` statement in the middle of the method body. Please set the value of the variable and return it at the end of the method.
   
   done




----------------------------------------------------------------
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

[GitHub] [hudi] yanghua commented on pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#issuecomment-674730151


   @wangxianghu conflicts should be fixed.


----------------------------------------------------------------
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

[GitHub] [hudi] yanghua commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r471267293



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/TimestampBasedKeyGenerator.java
##########
@@ -170,6 +173,32 @@ public String getPartitionPath(GenericRecord record) {
     }
   }
 
+  /**
+   * Set default value to partitionVal if the input value of partitionPathField is null.
+   */
+  private Object getDefaultPartitionVal() {
+    Object result = 1L;

Review comment:
       If you define a variable named `result`, then stop using `return` statement in the middle of the method body. Please set the value of the variable and return it at the end of the method.




----------------------------------------------------------------
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

[GitHub] [hudi] Mathieu1124 commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
Mathieu1124 commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r465826269



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/parser/HoodieDateTimeParserImpl.java
##########
@@ -58,7 +58,7 @@ public HoodieDateTimeParserImpl(TypedProperties config) {
     }
   }
 
-  private String getConfigInputDateFormatDelimiter() {
+  public String getConfigInputDateFormatDelimiter() {

Review comment:
       > can we make this package-private?
   
   @pratyakshsharma thanks for your review.
   I tried protected, it's not enough.




----------------------------------------------------------------
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

[GitHub] [hudi] pratyakshsharma commented on a change in pull request #1920: [HUDI-1150]Fix unable to parse input partition field :1 exception whe…

Posted by GitBox <gi...@apache.org>.
pratyakshsharma commented on a change in pull request #1920:
URL: https://github.com/apache/hudi/pull/1920#discussion_r465762652



##########
File path: hudi-spark/src/main/java/org/apache/hudi/keygen/parser/HoodieDateTimeParserImpl.java
##########
@@ -58,7 +58,7 @@ public HoodieDateTimeParserImpl(TypedProperties config) {
     }
   }
 
-  private String getConfigInputDateFormatDelimiter() {
+  public String getConfigInputDateFormatDelimiter() {

Review comment:
       can we make this package-private?




----------------------------------------------------------------
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