[GitHub] carbondata pull request #2404: [CARBONDATA-2634][BloomDataMap] Add datamap p...

[qiuchenjian-2]

GitHub user xuchuanyin opened a pull request:

    https://github.com/apache/carbondata/pull/2404

    [CARBONDATA-2634][BloomDataMap] Add datamap properties in show datamap outputs

    add datamap properties in show datamap outputs
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [x] Any interfaces changed?
     `NO`
     - [x] Any backward compatibility impacted?
     `NO`
     - [x] Document update required?
    `NO`
     - [x] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
    `Tests added`
            - How it is tested? Please attach test report.
    `Tested in local machine`
            - Is it a performance related change? Please attach the performance test report.
    `NO`
            - Any additional information to help reviewers in testing this change.
    `NA`
           
     - [x] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
    `NA`


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/xuchuanyin/carbondata 0624_show_dm_properties

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2404.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2404
   
----
commit 5cea0eaaa2ec1e3008839816248aed284ab790e0
Author: xuchuanyin <xuchuanyin@...>
Date:   2018-06-24T03:49:27Z

    Add datamap properties in show datamap outputs
   
    add datamap properties in show datamap outputs

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5412/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6491/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5322/



---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6507/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5339/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r197682568
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/datamap/TestDataMapCommand.scala ---
    @@ -204,6 +204,37 @@ class TestDataMapCommand extends QueryTest with BeforeAndAfterAll {
         }
       }
     
    +  test("test show datamap: show datamap property related information") {
    --- End diff --
   
    add a test case for preagg also, with DM properties and without DMProperties behavior


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r197682680
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/datamap/TestDataMapCommand.scala ---
    @@ -204,6 +204,37 @@ class TestDataMapCommand extends QueryTest with BeforeAndAfterAll {
         }
       }
     
    +  test("test show datamap: show datamap property related information") {
    +    val tableName = "datamapshowtest"
    +    val datamapName = "bloomdatamap"
    +    val datamapName2 = "bloomdatamap2"
    +    val datamapName3 = "bloomdatamap3"
    +    sql(s"drop table if exists $tableName")
    +    sql(s"create table $tableName (a string, b string, c string) stored by 'carbondata'")
    --- End diff --
   
    add a test case for preagg also, with DM properties and without DMProperties behavior


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r197810030
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/datamap/TestDataMapCommand.scala ---
    @@ -204,6 +204,37 @@ class TestDataMapCommand extends QueryTest with BeforeAndAfterAll {
         }
       }
     
    +  test("test show datamap: show datamap property related information") {
    +    val tableName = "datamapshowtest"
    +    val datamapName = "bloomdatamap"
    +    val datamapName2 = "bloomdatamap2"
    +    val datamapName3 = "bloomdatamap3"
    +    sql(s"drop table if exists $tableName")
    +    sql(s"create table $tableName (a string, b string, c string) stored by 'carbondata'")
    --- End diff --
   
    It seems that preaggregate datamap does not support user specified DMProperties. Howwever it has DMProperties internally.
    In the current commit, it will show:
    ```
    +-----------+------------+--------------------------------+----------------------------------------------------------------------------------------------------------------------------------------+
    |DataMapName|ClassName   |Associated Table                |DataMap Properties                                                                                                                      |
    +-----------+------------+--------------------------------+----------------------------------------------------------------------------------------------------------------------------------------+
    |datamap1   |preaggregate|default.datamapshowtest_datamap1|'CHILD_SELECT QUERY'='c2VsZWN0IGNvdW50KGEpIGZyb20gZGF0YW1hcHNob3d0ZXN0', 'QUERYTYPE'='AGGREGATION', '_internal.deferred.rebuild'='false'|
    +-----------+------------+--------------------------------+----------------------------------------------------------------------------------------------------------------------------------------+
    ```
   
    Do you think it's OK?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r197816298
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/datamap/TestDataMapCommand.scala ---
    @@ -204,6 +204,37 @@ class TestDataMapCommand extends QueryTest with BeforeAndAfterAll {
         }
       }
     
    +  test("test show datamap: show datamap property related information") {
    +    val tableName = "datamapshowtest"
    +    val datamapName = "bloomdatamap"
    +    val datamapName2 = "bloomdatamap2"
    +    val datamapName3 = "bloomdatamap3"
    +    sql(s"drop table if exists $tableName")
    +    sql(s"create table $tableName (a string, b string, c string) stored by 'carbondata'")
    --- End diff --
   
    no, we should not print the child select query to user, if DM properties is not given for preagg then you can show null, as we do in describe formatted for comment and all, which is the behavior of hive also.


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    @akashrn5 comments are resolved. only show user specified DMProperty in the output


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5376/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6548/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r198056881
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonDataMapShowCommand.scala ---
    @@ -66,11 +67,24 @@ case class CarbonDataMapShowCommand(tableIdentifier: Option[TableIdentifier])
     
       private def convertToRow(schemaList: util.List[DataMapSchema]) = {
         if (schemaList != null && schemaList.size() > 0) {
    -      schemaList.asScala.map { s =>
    -        var table = "(NA)"
    -        val relationIdentifier = s.getRelationIdentifier
    -          table = relationIdentifier.getDatabaseName + "." + relationIdentifier.getTableName
    -        Row(s.getDataMapName, s.getProviderName, table)
    +      schemaList.asScala
    +        .map { s =>
    +          val relationIdentifier = s.getRelationIdentifier
    +          val table = relationIdentifier.getDatabaseName + "." + relationIdentifier.getTableName
    +          // preaggregate datamap does not support user specified property, therefor we return empty
    +          val dmPropertieStr = if (s.getProviderName.equalsIgnoreCase(
    +              DataMapClassProvider.PREAGGREGATE.getShortName)) {
    --- End diff --
   
    How about timeseries and MV datamap?


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5451/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6550/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5379/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2404#discussion_r198120470
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonDataMapShowCommand.scala ---
    @@ -66,11 +67,24 @@ case class CarbonDataMapShowCommand(tableIdentifier: Option[TableIdentifier])
     
       private def convertToRow(schemaList: util.List[DataMapSchema]) = {
         if (schemaList != null && schemaList.size() > 0) {
    -      schemaList.asScala.map { s =>
    -        var table = "(NA)"
    -        val relationIdentifier = s.getRelationIdentifier
    -          table = relationIdentifier.getDatabaseName + "." + relationIdentifier.getTableName
    -        Row(s.getDataMapName, s.getProviderName, table)
    +      schemaList.asScala
    +        .map { s =>
    +          val relationIdentifier = s.getRelationIdentifier
    +          val table = relationIdentifier.getDatabaseName + "." + relationIdentifier.getTableName
    +          // preaggregate datamap does not support user specified property, therefor we return empty
    +          val dmPropertieStr = if (s.getProviderName.equalsIgnoreCase(
    +              DataMapClassProvider.PREAGGREGATE.getShortName)) {
    --- End diff --
   
    Timeseries datamap is OK, please refer to the added test.
   
    MV datamap currently is not available in this test case. When I added MV related test, the test will fail which complains that it cannot find MV


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2404
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5454/



---