xushiyan commented on code in PR #7901:
URL: https://github.com/apache/hudi/pull/7901#discussion_r1113711166


##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/DataSourceOptions.scala:
##########
@@ -830,6 +830,33 @@ object DataSourceOptionsHelper {
     translatedOpt.toMap
   }
 
+  /**
+   * Some config keys differ from what user sets and whats part of table 
Config. this method assists in fetching the
+   * right table config and populating write configs.
+   * @param tableConfig table config of interest.
+   * @param params incoming write params.
+   * @return missing params that needs to be added to incoming write params
+   */
+  def loadParamsFromTableConfig(tableConfig: HoodieTableConfig, params: 
Map[String, String]) : Map[String, String] = {

Review Comment:
   the method reads like loading straight from table config but in fact it's 
conditioned on input params. we should separate the loading from merging, or 
name the method properly



##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala:
##########
@@ -121,15 +122,19 @@ object HoodieSparkSqlWriter {
     val fs = basePath.getFileSystem(sparkContext.hadoopConfiguration)
     tableExists = fs.exists(new Path(basePath, 
HoodieTableMetaClient.METAFOLDER_NAME))
     var tableConfig = getHoodieTableConfig(sparkContext, path, 
hoodieTableConfigOpt)
-    val (parameters, hoodieConfig) = mergeParamsAndGetHoodieConfig(optParams, 
tableConfig, mode)
-    val originKeyGeneratorClassName = 
HoodieWriterUtils.getOriginKeyGenerator(parameters)
+    // get initial params and validate configs
+    val initialRawParams = getInitialParams(optParams)

Review Comment:
   I found names like `getInitialParams()`, `initialRawParams` and 
`getRawInitialParams()` pretty unclear to explain what the "stage" the params 
are. i suggest look for existing examples in the codebase and stick with the 
same naming patterns. on top of my head, i've seen `optParams` is the original 
user-input params, there is `finalOptParams`. Variables in this part worth 
spending some time crafting good names.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to