[GitHub] [carbondata] Indhumathi27 opened a new pull request #3786: [CARBONDATA-3842] Fix incorrect results on mv with limit (Missed code during mv refcatory)

[GitBox]

Indhumathi27 opened a new pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786


    ### Why is this PR needed?
    Already issue fixed in [PR-3652](https://github.com/apache/carbondata/pull/3651). Missed code during mv code refactory
   
    ### What changes were proposed in this PR?
   Copy subsume Flag and FlagSpec to subsumerPlan while rewriting with summarydatasets.
   Update the flagSpec as per the mv attributes and copy to relation.
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   
       
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-638066931


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1403/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-638067919


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3127/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

akashrn5 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-643985798


   @Indhumathi27 why the test cases didnt catch this. If we do not have test cases, please add a test case for the limit case.


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

akashrn5 commented on a change in pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#discussion_r440014735



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestAllOperationsOnMV.scala
##########
@@ -610,10 +610,10 @@ class TestAllOperationsOnMV extends QueryTest with BeforeAndAfterEach {
     sql("drop materialized view if exists mv1")
     sql("create materialized view mv1  as select a.name,a.price from maintable a")
     var dataFrame = sql("select a.name,a.price from maintable a limit 1")
-    assert(dataFrame.count() == 1)

Review comment:
       why this change? because count is also an action, it should also give 1




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#discussion_r440032092



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestAllOperationsOnMV.scala
##########
@@ -610,10 +610,10 @@ class TestAllOperationsOnMV extends QueryTest with BeforeAndAfterEach {
     sql("drop materialized view if exists mv1")
     sql("create materialized view mv1  as select a.name,a.price from maintable a")
     var dataFrame = sql("select a.name,a.price from maintable a limit 1")
-    assert(dataFrame.count() == 1)

Review comment:
       In this Case, count was returning 1 only, but df.show() is giving all results. In cluster also, limit query gives all rows. Because of this, testcase didnt catch this issue. That's why changed to df.collect.length




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#discussion_r440035757



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestAllOperationsOnMV.scala
##########
@@ -610,10 +610,10 @@ class TestAllOperationsOnMV extends QueryTest with BeforeAndAfterEach {
     sql("drop materialized view if exists mv1")
     sql("create materialized view mv1  as select a.name,a.price from maintable a")
     var dataFrame = sql("select a.name,a.price from maintable a limit 1")
-    assert(dataFrame.count() == 1)

Review comment:
       Here, count API is not applying collect rows on SPARK PLAN. When i checked it, plan was on main table




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#discussion_r440035757



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestAllOperationsOnMV.scala
##########
@@ -610,10 +610,10 @@ class TestAllOperationsOnMV extends QueryTest with BeforeAndAfterEach {
     sql("drop materialized view if exists mv1")
     sql("create materialized view mv1  as select a.name,a.price from maintable a")
     var dataFrame = sql("select a.name,a.price from maintable a limit 1")
-    assert(dataFrame.count() == 1)

Review comment:
       Here, count API is not applying collect rows on SPARK PHYSICAL PLAN. When i checked it, plan was on main table




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-653605007


   retest this please


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#discussion_r449642666



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestAllOperationsOnMV.scala
##########
@@ -610,10 +610,10 @@ class TestAllOperationsOnMV extends QueryTest with BeforeAndAfterEach {
     sql("drop materialized view if exists mv1")
     sql("create materialized view mv1  as select a.name,a.price from maintable a")
     var dataFrame = sql("select a.name,a.price from maintable a limit 1")
-    assert(dataFrame.count() == 1)

Review comment:
       @akashrn5 Please help to review and merge




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-653640139


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1563/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-653640599


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3300/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

akashrn5 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-654788035


   retest this please


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-654870645


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1578/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-654872715


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3316/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

akashrn5 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-655912151


   retest this please


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-655968434


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3335/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-655971860


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1595/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

akashrn5 commented on pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786#issuecomment-655974118


   LGTM


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

asfgit closed pull request #3786:
URL: https://github.com/apache/carbondata/pull/3786


   


----------------------------------------------------------------
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:
[hidden email]