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/03/24 01:00:59 UTC

[GitHub] [incubator-hudi] xushiyan opened a new pull request #1440: [HUDI-731] Add ChainedTransformer

xushiyan opened a new pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440
 
 
   ## *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
   
   *(For example: This pull request adds quick-start document.)*
   
   ## Brief change log
   
   *(for example:)*
     - *Modify AnnotationLocation checkstyle rule in checkstyle.xml*
   
   ## Verify this pull request
   
   *(Please pick either of the following options)*
   
   This pull request is a trivial rework / code cleanup without any test coverage.
   
   *(or)*
   
   This pull request is already covered by existing tests, such as *(please describe tests)*.
   
   (or)
   
   This change added tests and can be verified as follows:
   
   *(example:)*
   
     - *Added integration tests for end-to-end.*
     - *Added HoodieClientWriteTest to verify the change.*
     - *Manually verified the change by running a job locally.*
   
   ## 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/ce0a4c64d07d6eea926d1bfb92b69ae387b88f50&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `80.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1440      +/-   ##
   ============================================
   - Coverage     67.70%   67.64%   -0.06%     
   - Complexity      261      266       +5     
   ============================================
     Files           348      349       +1     
     Lines         16683    16689       +6     
     Branches       1694     1699       +5     
   ============================================
   - Hits          11295    11290       -5     
   - Misses         4647     4658      +11     
     Partials        741      741              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `78.60% <20.00%> (-1.60%)` | `8.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `65.41% <100.00%> (+2.62%)` | `21.00 <3.00> (+1.00)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `72.00% <100.00%> (-0.28%)` | `38.00 <0.00> (ø)` | |
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `4.00 <4.00> (?)` | |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `40.00% <0.00%> (-40.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `58.33% <0.00%> (-13.89%)` | `0.00% <0.00%> (ø%)` | |
   | [...ache/hudi/common/fs/inline/InMemoryFileSystem.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2ZzL2lubGluZS9Jbk1lbW9yeUZpbGVTeXN0ZW0uamF2YQ==) | `79.31% <0.00%> (-10.35%)` | `0.00% <0.00%> (ø%)` | |
   | [...e/compact/strategy/DayBasedCompactionStrategy.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdGFibGUvY29tcGFjdC9zdHJhdGVneS9EYXlCYXNlZENvbXBhY3Rpb25TdHJhdGVneS5qYXZh) | `65.00% <0.00%> (-1.67%)` | `0.00% <0.00%> (ø%)` | |
   | [...ava/org/apache/hudi/common/model/HoodieRecord.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZVJlY29yZC5qYXZh) | `81.03% <0.00%> (-1.51%)` | `0.00% <0.00%> (ø%)` | |
   | [...org/apache/hudi/config/HoodieHBaseIndexConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZUhCYXNlSW5kZXhDb25maWcuamF2YQ==) | `45.94% <0.00%> (-0.73%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [22 more](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [ce0a4c6...2510e16](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399821836
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   @vinothchandar jcommander converter feature seems to be the right fit. ~Guess we can change this variable to `Transformer` and get rid of `UtilHelpers.createTransformer()`. Sounds good?~ Actually for serializable config, let's just make it list of strings.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r400674693
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
 
 Review comment:
   Yes, I am a bit worried about these issues you mentioned. Another concern is the issue of conventions. Most test classes currently include Java docs, even though it seems they can describe themselves. Of course, this is another subjective issue worth considering. OK, here I no longer require Java doc.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399821332
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   let me check that.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399768961
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
 ##########
 @@ -102,11 +104,23 @@ public static SchemaProvider createSchemaProvider(String schemaProviderClass, Ty
     }
   }
 
-  public static Transformer createTransformer(String transformerClass) throws IOException {
+  public static Transformer createTransformer(String transformerClasses) throws IOException {
 
 Review comment:
   Can we add a CLI test case for arg `--transformer-class` so that we can cover the change of this 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399823355
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,63 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   Raised #1464 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399908243
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,92 @@
+/*
+ * 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.utilities.transform;
+
+import org.apache.hudi.utilities.UtilHelpers;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.types.DataTypes;
+import org.apache.spark.sql.types.StructField;
+import org.apache.spark.sql.types.StructType;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.apache.spark.sql.types.DataTypes.IntegerType;
+import static org.apache.spark.sql.types.DataTypes.StringType;
+import static org.apache.spark.sql.types.DataTypes.createStructField;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+
+public class TestChainedTransformer {
 
 Review comment:
   Java doc

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396894929
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
+
+import org.apache.hudi.common.util.TypedProperties;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import java.util.List;
+import java.util.function.Supplier;
+
+/**
+ * An abstract {@link Transformer} to chain other {@link Transformer}s and apply sequentially.
+ * <p>
+ * A subclass is to supply a {@link List} of {@link Transformer}s in desired sequence.
+ */
+public abstract class ChainedTransformer implements Transformer {
 
 Review comment:
   will do the docs change once this merges. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-607313717
 
 
   thanks @xushiyan , merging...

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r400558170
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
+
+  public static class TestCreateTransformer {
 
 Review comment:
   @yanghua That's the intended usage of `@RunWith(Enclosed.class)`, which is to run enclosed static classes separately. I think we should logically segregate tests for different utility methods in `UtilHelpers`, due to its versatility. E.g., only `TestCreateTransformer` should know about `TransformerFoo.class` and `TransformerBar.class` hence they are enclosed in this static class so that other test cases don't worry about them. Sounds reasonable?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-607291746
 
 
   @vinothchandar @yanghua Please check the last commit and merge if it's ok. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399818612
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,63 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   Guess rat check only works for source files.. we should fix that right away.. 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396879048
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   We can add a new section in the[ write documentation page](http://hudi.apache.org/docs/writing_data.html). The branch of documentation is `asf-site`. So it's OK to describe it in another PR.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/cafc87041baf4055c39244e7cde0187437bb03d4&el=desc) will **decrease** coverage by `0.00%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1440      +/-   ##
   ============================================
   - Coverage     67.61%   67.60%   -0.01%     
   - Complexity      254      257       +3     
   ============================================
     Files           340      341       +1     
     Lines         16504    16510       +6     
     Branches       1689     1690       +1     
   ============================================
   + Hits          11159    11162       +3     
   - Misses         4606     4609       +3     
     Partials        739      739              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `3.00 <3.00> (?)` | |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `40.00% <0.00%> (-60.00%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [cafc870...c2f9c32](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396894929
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
+
+import org.apache.hudi.common.util.TypedProperties;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import java.util.List;
+import java.util.function.Supplier;
+
+/**
+ * An abstract {@link Transformer} to chain other {@link Transformer}s and apply sequentially.
+ * <p>
+ * A subclass is to supply a {@link List} of {@link Transformer}s in desired sequence.
+ */
+public abstract class ChainedTransformer implements Transformer {
 
 Review comment:
   will do the docs change once this approved. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/ce0a4c64d07d6eea926d1bfb92b69ae387b88f50&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `80.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1440      +/-   ##
   ============================================
   - Coverage     67.70%   67.64%   -0.06%     
   - Complexity      261      266       +5     
   ============================================
     Files           348      349       +1     
     Lines         16683    16689       +6     
     Branches       1694     1699       +5     
   ============================================
   - Hits          11295    11290       -5     
   - Misses         4647     4658      +11     
     Partials        741      741              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `78.60% <20.00%> (-1.60%)` | `8.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `65.41% <100.00%> (+2.62%)` | `21.00 <3.00> (+1.00)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `72.00% <100.00%> (-0.28%)` | `38.00 <0.00> (ø)` | |
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `4.00 <4.00> (?)` | |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `40.00% <0.00%> (-40.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `58.33% <0.00%> (-13.89%)` | `0.00% <0.00%> (ø%)` | |
   | [...ache/hudi/common/fs/inline/InMemoryFileSystem.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2ZzL2lubGluZS9Jbk1lbW9yeUZpbGVTeXN0ZW0uamF2YQ==) | `79.31% <0.00%> (-10.35%)` | `0.00% <0.00%> (ø%)` | |
   | [...e/compact/strategy/DayBasedCompactionStrategy.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdGFibGUvY29tcGFjdC9zdHJhdGVneS9EYXlCYXNlZENvbXBhY3Rpb25TdHJhdGVneS5qYXZh) | `65.00% <0.00%> (-1.67%)` | `0.00% <0.00%> (ø%)` | |
   | [...ava/org/apache/hudi/common/model/HoodieRecord.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZVJlY29yZC5qYXZh) | `81.03% <0.00%> (-1.51%)` | `0.00% <0.00%> (ø%)` | |
   | [...org/apache/hudi/config/HoodieHBaseIndexConfig.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29uZmlnL0hvb2RpZUhCYXNlSW5kZXhDb25maWcuamF2YQ==) | `45.94% <0.00%> (-0.73%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [22 more](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [ce0a4c6...2510e16](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399820683
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
 ##########
 @@ -102,11 +104,23 @@ public static SchemaProvider createSchemaProvider(String schemaProviderClass, Ty
     }
   }
 
-  public static Transformer createTransformer(String transformerClass) throws IOException {
+  public static Transformer createTransformer(String transformerClasses) throws IOException {
 
 Review comment:
   ok 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399821836
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   @vinothchandar jcommander converter feature seems to be the right fit. Guess we can change this variable to `List<Transformer>` and get rid of `UtilHelpers.createTransformer()`. Sounds good?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-603012975
 
 
   @vinothchandar The current approach is also compatible with existing CLI usage. The user extends this abstract class and plugs that custom class to `--transformer-class` param. So the difference is: transformer list is supplied from user's code instead of being parsed from CLI args. 
   
   My reasoning for this implementation is: it saves some parsing logic to maintain and users could have more flexibility in how they supply transformers (via their own code).
   
   Though saying that, I don't mind making the transformers parsed from CLI. It's kinda user experience trade-off.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396869939
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
+
+import org.apache.hudi.common.util.TypedProperties;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import java.util.List;
+import java.util.function.Supplier;
+
+/**
+ * An abstract {@link Transformer} to chain other {@link Transformer}s and apply sequentially.
+ * <p>
+ * A subclass is to supply a {@link List} of {@link Transformer}s in desired sequence.
+ */
+public abstract class ChainedTransformer implements Transformer {
+
+  protected abstract Supplier<List<Transformer>> supplyTransformers();
+
+  @Override
+  public Dataset<Row> apply(JavaSparkContext jsc, SparkSession sparkSession, Dataset<Row> rowDataset, TypedProperties properties) {
 
 Review comment:
   It would be better to have a test case which extends `ChainedTransformer ` and mock some transformer to verify this function.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399821836
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   @vinothchandar jcommander converter feature seems to be the right fit. Guess we can change this variable to `Transformer` and get rid of `UtilHelpers.createTransformer()`. Sounds good?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399811247
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,63 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   Good catch! @xushiyan , you can configure your IDE to add a file header template. For example, Intellij IDEA does support this feature. @leesf Can we let Travis check the license header?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399818801
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   jcommander supports parsing a list of csvs into a List<String> here already. could we switch to that

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-603045516
 
 
    @xushiyan 's explanation is correct. It's just about trade-off. Both of them can achieve the same function, but the mechanism of collecting `Transformer` is different. Considering that we have limited the functionality of this `Transformer` to: `ChainedTransformer`. Maybe we can hide the "collect/assemble" logic to users? From this perspective, perhaps @vinothchandar 's suggestion is a little better for users. 
   
   After all, in terms of flexibility, if users want more flexibility, they can even directly inherit the `Transformer` interface and implement everything you currently implement, right?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r400554146
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
 
 Review comment:
   @yanghua I doubt the benefits of adding java doc to unit test classes. The package path and the class name have very well pinpointed the class under tests. Plus the test case method names are self-explanatory, we don't want to put redundant info in a comment block, do we? If you're concerned with the checkstyle warnings, maybe we should consider exclude this warning from test sources. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399818837
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
 ##########
 @@ -102,11 +104,23 @@ public static SchemaProvider createSchemaProvider(String schemaProviderClass, Ty
     }
   }
 
-  public static Transformer createTransformer(String transformerClass) throws IOException {
+  public static Transformer createTransformer(String transformerClasses) throws IOException {
     try {
-      return transformerClass == null ? null : (Transformer) ReflectionUtils.loadClass(transformerClass);
+      if (transformerClasses == null) {
+        return null;
+      }
+
+      String[] classNames = Arrays.stream(transformerClasses.split(","))
 
 Review comment:
   this logic can be pushed to command line parsing?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r400990823
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
 ##########
 @@ -102,11 +104,19 @@ public static SchemaProvider createSchemaProvider(String schemaProviderClass, Ty
     }
   }
 
-  public static Transformer createTransformer(String transformerClass) throws IOException {
+  public static Transformer createTransformer(List<String> classNames) throws IOException {
     try {
-      return transformerClass == null ? null : (Transformer) ReflectionUtils.loadClass(transformerClass);
+      if (classNames == null) {
 
 Review comment:
   let's use Optional instead of null.. NPE are pesky :) 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399907865
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
 
 Review comment:
   It would be better to add java docs for the test class.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua merged pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua merged pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440
 
 
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/cafc87041baf4055c39244e7cde0187437bb03d4&el=desc) will **increase** coverage by `0.00%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #1440   +/-   ##
   =========================================
     Coverage     67.61%   67.61%           
     Complexity      254      254           
   =========================================
     Files           340      341    +1     
     Lines         16504    16510    +6     
     Branches       1689     1690    +1     
   =========================================
   + Hits          11159    11164    +5     
   - Misses         4606     4607    +1     
     Partials        739      739           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `70.27% <0.00%> (+13.51%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [cafc870...81b3b0c](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/ac73bdcdc39e100d4082cf54359690490e5774f7&el=desc) will **increase** coverage by `0.07%`.
   > The diff coverage is `90.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1440      +/-   ##
   ============================================
   + Coverage     67.55%   67.63%   +0.07%     
   - Complexity      261      265       +4     
   ============================================
     Files           347      348       +1     
     Lines         16659    16676      +17     
     Branches       1693     1695       +2     
   ============================================
   + Hits          11254    11278      +24     
   + Misses         4666     4658       -8     
   - Partials        739      740       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `80.20% <ø> (ø)` | `8.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `64.49% <81.81%> (+1.70%)` | `23.00 <5.00> (+3.00)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `72.27% <100.00%> (ø)` | `38.00 <0.00> (ø)` | |
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `3.00 <3.00> (?)` | |
   | [...che/hudi/common/util/BufferedRandomAccessFile.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQnVmZmVyZWRSYW5kb21BY2Nlc3NGaWxlLmphdmE=) | `55.26% <0.00%> (+0.87%)` | `0.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `72.22% <0.00%> (+13.88%)` | `0.00% <0.00%> (ø%)` | |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `80.00% <0.00%> (+40.00%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [ac73bdc...93f18a1](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] leesf commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
leesf commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399793162
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,63 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   Missing Apache Header.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-603417382
 
 
   @vinothchandar @yanghua Sure thanks for the input. Since the parsing approach saves some efforts for users, I can make it that way. May I have your 👍 on this comment to get the green light to work on it?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-603901546
 
 
   sorry for dropping off.. crazy times :/ 
   
   I agree with the tradeoffs and points discussed here.. Even though users can subclass the new class, then it adds documentation/discoverability overheads for the user.. UX level, it may be good to just turn our existing param into taking a list of class names and use this class internally.. 
   
   +1 on reworking towards that.. thanks @xushiyan 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r400675355
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
+
+  public static class TestCreateTransformer {
 
 Review comment:
   Ok, reasonable.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396869571
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   This class missed the Apache license header. And it's strange why the Travis is green? Does it not check this rule? cc @vinothchandar 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399820497
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/transform/TestChainedTransformer.java
 ##########
 @@ -0,0 +1,63 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   Issue actually is the phase I think.. I locally tested that clean install /clean package will flag a file without header.. 
   ```
    <executions>
               <execution>
                 <phase>package</phase>
                 <goals>
                   <goal>check</goal>
                 </goals>
               </execution>
             </executions>
   ```
   
   we need to change this to `verify` probably.. 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396873427
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
 
 Review comment:
   @yanghua Fixing it.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-605593208
 
 
   @yanghua @vinothchandar please check the last commit for the rework. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399823463
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java
 ##########
 @@ -195,11 +195,12 @@ public Operation convert(String value) throws ParameterException {
     public String schemaProviderClassName = null;
 
     @Parameter(names = {"--transformer-class"},
-        description = "subclass of org.apache.hudi.utilities.transform.Transformer"
+        description = "A subclass or a list of subclasses of org.apache.hudi.utilities.transform.Transformer"
             + ". Allows transforming raw source Dataset to a target Dataset (conforming to target schema) before "
             + "writing. Default : Not set. E:g - org.apache.hudi.utilities.transform.SqlQueryBasedTransformer (which "
-            + "allows a SQL query templated to be passed as a transformation function)")
-    public String transformerClassName = null;
+            + "allows a SQL query templated to be passed as a transformation function). "
+            + "Pass a comma-separated list of subclass names to chain the transformations.")
+    public String transformerClassNames = null;
 
 Review comment:
   yes :) .. 

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396873391
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
+
+import org.apache.hudi.common.util.TypedProperties;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import java.util.List;
+import java.util.function.Supplier;
+
+/**
+ * An abstract {@link Transformer} to chain other {@link Transformer}s and apply sequentially.
+ * <p>
+ * A subclass is to supply a {@link List} of {@link Transformer}s in desired sequence.
+ */
+public abstract class ChainedTransformer implements Transformer {
 
 Review comment:
   Sure @yanghua . Could you point me to where I can add this?

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/ac73bdcdc39e100d4082cf54359690490e5774f7&el=desc) will **increase** coverage by `0.07%`.
   > The diff coverage is `90.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1440      +/-   ##
   ============================================
   + Coverage     67.55%   67.63%   +0.07%     
   - Complexity      261      265       +4     
   ============================================
     Files           347      348       +1     
     Lines         16659    16676      +17     
     Branches       1693     1695       +2     
   ============================================
   + Hits          11254    11278      +24     
   + Misses         4666     4658       -8     
   - Partials        739      740       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `80.20% <ø> (ø)` | `8.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `64.49% <81.81%> (+1.70%)` | `23.00 <5.00> (+3.00)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `72.27% <100.00%> (ø)` | `38.00 <0.00> (ø)` | |
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `3.00 <3.00> (?)` | |
   | [...che/hudi/common/util/BufferedRandomAccessFile.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQnVmZmVyZWRSYW5kb21BY2Nlc3NGaWxlLmphdmE=) | `55.26% <0.00%> (+0.87%)` | `0.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `72.22% <0.00%> (+13.88%)` | `0.00% <0.00%> (ø%)` | |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `80.00% <0.00%> (+40.00%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [ac73bdc...93f18a1](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r396870236
 
 

 ##########
 File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/transform/ChainedTransformer.java
 ##########
 @@ -0,0 +1,30 @@
+package org.apache.hudi.utilities.transform;
+
+import org.apache.hudi.common.util.TypedProperties;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import java.util.List;
+import java.util.function.Supplier;
+
+/**
+ * An abstract {@link Transformer} to chain other {@link Transformer}s and apply sequentially.
+ * <p>
+ * A subclass is to supply a {@link List} of {@link Transformer}s in desired sequence.
+ */
+public abstract class ChainedTransformer implements Transformer {
 
 Review comment:
   It's a user-faced feature, we'd better to describe it in the documentation. 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#discussion_r399908175
 
 

 ##########
 File path: hudi-utilities/src/test/java/org/apache/hudi/utilities/TestUtilHelpers.java
 ##########
 @@ -0,0 +1,101 @@
+/*
+ * 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.utilities;
+
+import org.apache.hudi.common.config.TypedProperties;
+import org.apache.hudi.utilities.transform.ChainedTransformer;
+import org.apache.hudi.utilities.transform.Transformer;
+
+import org.apache.spark.api.java.JavaSparkContext;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.rules.ExpectedException;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+@RunWith(Enclosed.class)
+public class TestUtilHelpers {
+
+  public static class TestCreateTransformer {
 
 Review comment:
   IMO, it is not necessary to create this static class, all the test cases can be placed in the `TestUtilHelpers`.

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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io commented on issue #1440: [HUDI-731] Add ChainedTransformer

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1440: [HUDI-731] Add ChainedTransformer
URL: https://github.com/apache/incubator-hudi/pull/1440#issuecomment-602962209
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=h1) Report
   > Merging [#1440](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/cafc87041baf4055c39244e7cde0187437bb03d4&el=desc) will **increase** coverage by `0.00%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1440/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #1440   +/-   ##
   =========================================
     Coverage     67.61%   67.61%           
     Complexity      254      254           
   =========================================
     Files           340      341    +1     
     Lines         16504    16510    +6     
     Branches       1689     1690    +1     
   =========================================
   + Hits          11159    11164    +5     
   - Misses         4606     4607    +1     
     Partials        739      739           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...e/hudi/utilities/transform/ChainedTransformer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3RyYW5zZm9ybS9DaGFpbmVkVHJhbnNmb3JtZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
   | [...src/main/java/org/apache/hudi/metrics/Metrics.java](https://codecov.io/gh/apache/incubator-hudi/pull/1440/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9NZXRyaWNzLmphdmE=) | `70.27% <0.00%> (+13.51%)` | `0.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=footer). Last update [cafc870...81b3b0c](https://codecov.io/gh/apache/incubator-hudi/pull/1440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services