[GitHub] carbondata pull request #3045: [CARBONDATA-3222]Fix dataload failure after c...

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r244992105
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -126,6 +126,12 @@ case class PreAggregateTableHelper(
             newLongStringColumn.mkString(","))
         }
     
    +    //Add long_string_columns properties in child table from the parent.
    +    tableProperties
    --- End diff --
   
    Done. @kumarvishal09 please review.


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245272931
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    --- End diff --
   
    Please use isEmpty


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245273039
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    +        val relationList = (fields._2.columnTableRelationList)
    +        relationList.foreach(rel => {
    +          rel.foreach(col => {
    +            if (longStringColumnInParents.contains(col.parentColumnName)) {
    +              varcharDatamapFields += col.parentColumnName + ","
    +            }
    +          })
    +        })
    +      }
    +    })
    +    if (varcharDatamapFields.size != 0) {
    +      longStringColumn = Option(varcharDatamapFields.slice(0, varcharDatamapFields.length - 1))
    +    }
         if (longStringColumn != None) {
           val fieldNames = fields.map(_.column)
    -      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map{ colName =>
    +      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map { colName =>
             val newColName = parentTable.getTableName.toLowerCase() + "_" + colName
             if (!fieldNames.contains(newColName)) {
               throw new MalformedDataMapCommandException(
                 CarbonCommonConstants.LONG_STRING_COLUMNS.toUpperCase() + ":" + colName
    -              + " does not in datamap")
    +            + " does not in datamap")
    --- End diff --
   
    revery this change


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245274147
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    --- End diff --
   
    add if(fields._2.aggregateFunction.isEmpty && fields._2.columnTableRelationList.size==1 && longStringColumnInParents.contains(fields._2.columnTableRelationList.head.parentColumnName)) {
   
    }


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245274310
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    --- End diff --
   
    use seq instead of string


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245275437
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    +        val relationList = (fields._2.columnTableRelationList)
    +        relationList.foreach(rel => {
    +          rel.foreach(col => {
    +            if (longStringColumnInParents.contains(col.parentColumnName)) {
    +              varcharDatamapFields += col.parentColumnName + ","
    +            }
    +          })
    +        })
    +      }
    +    })
    +    if (varcharDatamapFields.size != 0) {
    --- End diff --
   
    remove this code, covert comma separated string from seq


---

[qiuchenjian-2]

Github user kumarvishal09 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245275753
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    +        val relationList = (fields._2.columnTableRelationList)
    +        relationList.foreach(rel => {
    +          rel.foreach(col => {
    +            if (longStringColumnInParents.contains(col.parentColumnName)) {
    +              varcharDatamapFields += col.parentColumnName + ","
    +            }
    +          })
    +        })
    +      }
    +    })
    +    if (varcharDatamapFields.size != 0) {
    +      longStringColumn = Option(varcharDatamapFields.slice(0, varcharDatamapFields.length - 1))
    +    }
         if (longStringColumn != None) {
           val fieldNames = fields.map(_.column)
    -      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map{ colName =>
    +      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map { colName =>
             val newColName = parentTable.getTableName.toLowerCase() + "_" + colName
             if (!fieldNames.contains(newColName)) {
               throw new MalformedDataMapCommandException(
                 CarbonCommonConstants.LONG_STRING_COLUMNS.toUpperCase() + ":" + colName
    -              + " does not in datamap")
    +            + " does not in datamap")
             }
             newColName
           }
           tableProperties.put(CarbonCommonConstants.LONG_STRING_COLUMNS,
             newLongStringColumn.mkString(","))
         }
    -
    --- End diff --
   
    revert this change


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3045
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2172/



---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245475409
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    +        val relationList = (fields._2.columnTableRelationList)
    +        relationList.foreach(rel => {
    +          rel.foreach(col => {
    +            if (longStringColumnInParents.contains(col.parentColumnName)) {
    +              varcharDatamapFields += col.parentColumnName + ","
    +            }
    +          })
    +        })
    +      }
    +    })
    +    if (varcharDatamapFields.size != 0) {
    --- End diff --
   
    Done!


---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245475427
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    --- End diff --
   
    Done!


---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245475435
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,22 +109,42 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dmproperties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    val longStringColumnInParents = parentTable.getTableInfo.getFactTable.getTableProperties.asScala
    +      .getOrElse(CarbonCommonConstants.LONG_STRING_COLUMNS, "").split(",").map(_.trim)
    +    var varcharDatamapFields = ""
    +    fieldRelationMap foreach (fields => {
    +      val aggFunc = fields._2.aggregateFunction
    +      if (aggFunc == "") {
    +        val relationList = (fields._2.columnTableRelationList)
    +        relationList.foreach(rel => {
    +          rel.foreach(col => {
    +            if (longStringColumnInParents.contains(col.parentColumnName)) {
    +              varcharDatamapFields += col.parentColumnName + ","
    +            }
    +          })
    +        })
    +      }
    +    })
    +    if (varcharDatamapFields.size != 0) {
    +      longStringColumn = Option(varcharDatamapFields.slice(0, varcharDatamapFields.length - 1))
    +    }
         if (longStringColumn != None) {
           val fieldNames = fields.map(_.column)
    -      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map{ colName =>
    +      val newLongStringColumn = longStringColumn.get.split(",").map(_.trim).map { colName =>
             val newColName = parentTable.getTableName.toLowerCase() + "_" + colName
             if (!fieldNames.contains(newColName)) {
               throw new MalformedDataMapCommandException(
                 CarbonCommonConstants.LONG_STRING_COLUMNS.toUpperCase() + ":" + colName
    -              + " does not in datamap")
    +            + " does not in datamap")
    --- End diff --
   
    Done!


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3045
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2387/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3045
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10428/



---

[qiuchenjian-2]

Github user xuchuanyin commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245509909
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,7 +110,29 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    --- End diff --
   
    emm, actually this line of code was added to solve the same problem mentioned in the PR description. Previously we thought it's user's responsibility to explicitly specify the long_string_columns in the preagg datamap.
   
    Please @kevinjmh also check this.


---

[qiuchenjian-2]

Github user kevinjmh commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245511008
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,7 +110,29 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dm properties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    --- End diff --
   
    So user do not have to config dmproperties for long string anymore and this line is no use after this PR?


---

[qiuchenjian-2]

Github user kevinjmh commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245511058
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,7 +110,29 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dm properties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    --- End diff --
   
    The codes is not consistent with comments in line 113. Please fix it



---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245516158
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,7 +110,29 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    +    // If longStringColumn is not present in dm properties then we take long_string_columns from
    +    // the parent table.
    +    var longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    --- End diff --
   
    Fixed it. If the user doesn't configure long_string in dmproperties, we take long_string_cols from the parent table.


---

[qiuchenjian-2]

Github user shardul-cr7 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/3045#discussion_r245516216
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateTableHelper.scala ---
    @@ -110,7 +110,29 @@ case class PreAggregateTableHelper(
         // Datamap table name and columns are automatically added prefix with parent table name
         // in carbon. For convenient, users can type column names same as the ones in select statement
         // when config dmproperties, and here we update column names with prefix.
    -    val longStringColumn = tableProperties.get(CarbonCommonConstants.LONG_STRING_COLUMNS)
    --- End diff --
   
    This PR is for the scenario when user doesn't config the long_string_columns in dmprop then we take long_string_cols from the parent table.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3045
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2175/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/3045
 
    Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10431/



---