[GitHub] carbondata pull request #2004: [CARBONDATA-2208]Pre aggregate datamap creati...

[qiuchenjian-2]

GitHub user kumarvishal09 opened a pull request:

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

    [CARBONDATA-2208]Pre aggregate datamap creation is failing when count(*) present in query

    Pre aggregate data map creation is failing with parsing error
   
    create datamap agg9 on table maintable using 'preaggregate' as select name, count from maintable group by name
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
           Added UT
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


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

    $ git pull https://github.com/kumarvishal09/incubator-carbondata master_26-02-2018

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

    https://github.com/apache/carbondata/pull/2004.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 #2004
   
----
commit 7b51a59148b570093fe82f3a14217c1c24886509
Author: kumarvishal <kumarvishal.1802@...>
Date:   2018-02-27T10:31:06Z

    Fixed count(*) query issue in pre aggregate

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170902884
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/path/CarbonTablePath.java ---
    @@ -75,7 +75,12 @@ public CarbonTablePath(CarbonTableIdentifier carbonTableIdentifier, String table
        * @param carbonFilePath
        */
       public static String getFolderContainingFile(String carbonFilePath) {
    -    return carbonFilePath.substring(0, carbonFilePath.lastIndexOf('/'));
    +    int lastIndex = carbonFilePath.lastIndexOf('/');
    +    // below code for handling windows environment
    +    if (-1 == lastIndex) {
    --- End diff --
   
    Whether it need support this scenario: E:/xubo\idea ?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170906653
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateUtil.scala ---
    @@ -346,11 +346,13 @@ object PreAggregateUtil {
             carbonTable)
         }
         // if parent column relation is of size more than one that means aggregate table
    -    // column is derived from multiple column of main table
    -    // or if expression is not a instance of attribute reference
    +    // column is derived from multiple column of main table or if size is zero then it means
    +    // column is present in select statement is some constants for example count(*)
    +    // and if expression is not a instance of attribute reference
         // then use column name which is passed
         val columnName =
    -    if (parentColumnsName.size > 1 && !expression.isInstanceOf[AttributeReference]) {
    +    if ((parentColumnsName.size > 1 || parentColumnsName.isEmpty) &&
    +        !expression.isInstanceOf[AttributeReference]) {
           newColumnName
    --- End diff --
   
    Why don't we use 1_count instead of 0_count in default?
    It is count(1)  after spark parser count(*)


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170908712
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateUtil.scala ---
    @@ -346,11 +346,13 @@ object PreAggregateUtil {
             carbonTable)
         }
         // if parent column relation is of size more than one that means aggregate table
    -    // column is derived from multiple column of main table
    -    // or if expression is not a instance of attribute reference
    +    // column is derived from multiple column of main table or if size is zero then it means
    +    // column is present in select statement is some constants for example count(*)
    +    // and if expression is not a instance of attribute reference
         // then use column name which is passed
         val columnName =
    -    if (parentColumnsName.size > 1 && !expression.isInstanceOf[AttributeReference]) {
    +    if ((parentColumnsName.size > 1 || parentColumnsName.isEmpty) &&
    +        !expression.isInstanceOf[AttributeReference]) {
           newColumnName
    --- End diff --
   
    i didn't get your comment


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170909437
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/path/CarbonTablePath.java ---
    @@ -75,7 +75,12 @@ public CarbonTablePath(CarbonTableIdentifier carbonTableIdentifier, String table
        * @param carbonFilePath
        */
       public static String getFolderContainingFile(String carbonFilePath) {
    -    return carbonFilePath.substring(0, carbonFilePath.lastIndexOf('/'));
    +    int lastIndex = carbonFilePath.lastIndexOf('/');
    +    // below code for handling windows environment
    +    if (-1 == lastIndex) {
    --- End diff --
   
    in windows file seperator is "\\" , this is to handle the same


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2004
 
    LGTM, verified it.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170916327
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/path/CarbonTablePath.java ---
    @@ -75,7 +75,12 @@ public CarbonTablePath(CarbonTableIdentifier carbonTableIdentifier, String table
        * @param carbonFilePath
        */
       public static String getFolderContainingFile(String carbonFilePath) {
    -    return carbonFilePath.substring(0, carbonFilePath.lastIndexOf('/'));
    +    int lastIndex = carbonFilePath.lastIndexOf('/');
    +    // below code for handling windows environment
    +    if (-1 == lastIndex) {
    --- End diff --
   
    in windows, you code will return E: if you input  E:/xubo\idea , not return E:/xubo


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170916613
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/path/CarbonTablePath.java ---
    @@ -75,7 +75,12 @@ public CarbonTablePath(CarbonTableIdentifier carbonTableIdentifier, String table
        * @param carbonFilePath
        */
       public static String getFolderContainingFile(String carbonFilePath) {
    -    return carbonFilePath.substring(0, carbonFilePath.lastIndexOf('/'));
    +    int lastIndex = carbonFilePath.lastIndexOf('/');
    +    // below code for handling windows environment
    +    if (-1 == lastIndex) {
    --- End diff --
   
     E:/xubo\idea  will never come either  it will be E:/xubo/idea or  E:\xubo\idea ?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170919785
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateUtil.scala ---
    @@ -346,11 +346,13 @@ object PreAggregateUtil {
             carbonTable)
         }
         // if parent column relation is of size more than one that means aggregate table
    -    // column is derived from multiple column of main table
    -    // or if expression is not a instance of attribute reference
    +    // column is derived from multiple column of main table or if size is zero then it means
    +    // column is present in select statement is some constants for example count(*)
    +    // and if expression is not a instance of attribute reference
         // then use column name which is passed
         val columnName =
    -    if (parentColumnsName.size > 1 && !expression.isInstanceOf[AttributeReference]) {
    +    if ((parentColumnsName.size > 1 || parentColumnsName.isEmpty) &&
    +        !expression.isInstanceOf[AttributeReference]) {
           newColumnName
    --- End diff --
   
    ok, it understand. It's fine, thanks.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2004#discussion_r170922780
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/path/CarbonTablePath.java ---
    @@ -75,7 +75,12 @@ public CarbonTablePath(CarbonTableIdentifier carbonTableIdentifier, String table
        * @param carbonFilePath
        */
       public static String getFolderContainingFile(String carbonFilePath) {
    -    return carbonFilePath.substring(0, carbonFilePath.lastIndexOf('/'));
    +    int lastIndex = carbonFilePath.lastIndexOf('/');
    +    // below code for handling windows environment
    +    if (-1 == lastIndex) {
    --- End diff --
   
    ok, it's fine


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---