[CARBONDATA-2989] Upgrade spark integration version to 2.3.2 #2779

zzcclp · 2018-09-27T17:32:15Z

Changes:

According to SPARK-PR#22346, change the parameter type from 'outputColumns: Seq[Attribute]' to 'outputColumnNames: Seq[String]' when call 'writeAndRead' method;
According to SPARK-PR#21815, there are some parameters added 'lazy', so move original class 'CarbonDataSourceScan' to src path 'commonTo2.1And2.2', and add a new class 'CarbonDataSourceScan' in src path 'spark2.3' which is added some lazy parameters.
Upgrade spark integration version to 2.3.2

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed? No
Any backward compatibility impacted? No
Document update required? No
Testing done
Please provide details on
- Whether new unit test cases have been added or why no new tests are required?
- How it is tested? Please attach test report.
- Is it a performance related change? Please attach the performance test report.
- Any additional information to help reviewers in testing this change.
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

zzcclp · 2018-09-27T17:36:21Z

integration/spark-common/src/main/scala/org/apache/spark/util/CarbonReflectionUtils.scala

          classOf[Seq[Attribute]],
          classOf[SparkPlan])
-      method.invoke(dataSourceObj, mode, query, query.output, physicalPlan)
+      method.invoke(dataSourceObj, mode, query, query.output.map(_.name), physicalPlan)


The parameters of 'writeAndRead' method had been changed, please see: SPARK-PR#22346

added some comments too

zzcclp · 2018-09-27T17:41:00Z

.../spark2/src/main/spark2.3/org/apache/spark/sql/execution/strategy/CarbonDataSourceScan.scala

+  override lazy val (outputPartitioning, outputOrdering): (Partitioning, Seq[SortOrder]) =
+    (partitioning, Nil)
+
+  override lazy val metadata: Map[String, String] = md


The parameters (supportsBatch, outputPartitioning, outputOrdering, metadata) had been added keyword 'lazy', please see: SPARK-PR#21815

nit: made lazy since spark 2.3.2 version (SPARK-PR#21815)

zzcclp · 2018-09-27T17:41:59Z

@jackylk @chenliang613 @sujith71955 please review.

CarbonDataQA · 2018-09-27T17:54:00Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/615/

sujith71955 · 2018-09-27T18:10:09Z

Thanks for raising the PR, It will better if you can add the description about the changes in this PR.

CarbonDataQA · 2018-09-27T19:43:05Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8876/

CarbonDataQA · 2018-09-27T19:43:05Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/807/

chenliang613 · 2018-09-28T02:50:15Z

retest this please

chenliang613 · 2018-09-28T03:10:29Z

.../spark2/src/main/spark2.3/org/apache/spark/sql/execution/strategy/CarbonDataSourceScan.scala

@@ -0,0 +1,55 @@
+/*


Why need to move CarbonDataSourceScan.scala?

move original class 'CarbonDataSourceScan' to src path 'commonTo2.1And2.2', and add a new class 'CarbonDataSourceScan' in src path 'spark2.3' which is added some lazy parameters.

My comment : only for 4 parameters , copy the whole file(CarbonDataSourceScan.scala) for spark 2.3 integration, may not require. see if can add the judgement for different spark version with different code/parameters.

@chenliang613 , it's not about the different code/parameters, it changes common val parameters to lazy val parameters. I think it's difficult to add lazy keyword on val parameters according to spark version.
Or does anyone know how to do and give me some help, thanks.

CarbonDataQA · 2018-09-28T03:43:04Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/619/

CarbonDataQA · 2018-09-28T04:43:04Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8880/

CarbonDataQA · 2018-09-28T04:43:05Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/811/

ravipesala · 2018-09-28T13:58:24Z

@zzcclp Please check and fix the tests

zzcclp · 2018-09-28T14:02:26Z

@ravipesala can you help me to check why these three test cases fail? It's about the decimal precision.

zzcclp · 2018-09-28T14:38:23Z

@ravipesala I know how to fix and will fix the tests ASAP.

Upgrade spark integration version to 2.3.2

CarbonDataQA · 2018-09-28T16:44:29Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/837/

CarbonDataQA · 2018-09-28T17:00:55Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/643/

CarbonDataQA · 2018-09-28T17:57:01Z

Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8905/

zzcclp · 2018-09-28T22:36:20Z

@sujith71955 @chenliang613 @ravipesala @jackylk this pr is ready, please review, thanks.

chenliang613 · 2018-09-29T04:19:24Z

My comment : only for 4 parameters , copy the whole file(CarbonDataSourceScan.scala) for spark 2.3 integration, may not require. see if can add the judgement for different spark version with different code/parameters.

jackylk · 2018-09-29T08:01:09Z

I think it is not possible to add lazy variable without breaking spark 2.2 integration. Since this class is small, I think it is ok to have a separate one for spark 2.3 integration

zzcclp · 2018-09-29T09:39:53Z

@sujith71955 any suggestion for this?

chenliang613 · 2018-09-30T01:36:26Z

LGTM

ravipesala · 2018-09-30T01:44:13Z

LGTM

sujith71955 · 2018-09-30T02:26:56Z

LGTM Even though SPARK-PR#21815 changes is a workaround solution and in future its subjected to change. Need a caution here. Can you please add this PR reference in the modified code of the carbon for future reference. I think you add like ' made lazy since spark 2.3.2 version(SPARK-PR#21815)
Thanks for your effort.

CarbonDataQA · 2018-09-30T04:13:02Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/656/

CarbonDataQA · 2018-09-30T06:13:02Z

Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8920/

CarbonDataQA · 2018-09-30T06:13:03Z

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/852/

zzcclp · 2018-09-30T06:46:15Z

the failed test case is not related to this pr, right?

sujith71955 · 2018-09-30T06:49:07Z

I think better to re-trigger once

sujith71955 · 2018-09-30T06:49:40Z

retest this please

sujith71955 · 2018-09-30T06:57:40Z

Seems to be impact of this PR , but it has passed previously,since we modified the outputpartition as lazy, better to have a relook once

zzcclp · 2018-09-30T07:14:14Z

ok, but it doesn't add lazy for spak 2.2.

sujith71955 · 2018-09-30T07:21:34Z

Right, independently run this case and check once. Also retrigger the build. Thanks

…

On Sun, 30 Sep 2018 at 12:44 PM, Zhichao Zhang ***@***.***> wrote: ok, but it doesn't add lazy for spak 2.2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2779 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMZZ-TP2bfR9iJTUhBM1yDJDUusA5VcDks5ugG9HgaJpZM4W9IBA> .

CarbonDataQA · 2018-09-30T07:23:19Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/660/

CarbonDataQA · 2018-09-30T08:26:17Z

Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/8923/

CarbonDataQA · 2018-09-30T08:28:31Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/855/

zzcclp mentioned this pull request Sep 27, 2018

[HOXFIX] Upgrade spark integration version to 2.3.2 #2777

Closed

zzcclp commented Sep 27, 2018

View reviewed changes

chenliang613 reviewed Sep 28, 2018

View reviewed changes

zzcclp changed the title ~~[WIP] Upgrade spark integration version to 2.3.2~~ [CARBONDATA-2989] Upgrade spark integration version to 2.3.2 Sep 28, 2018

zzcclp added 2 commits September 28, 2018 22:49

[WIP] Upgrade spark integration version to 2.3.2

e3163a4

Upgrade spark integration version to 2.3.2

fix below test cases

c8b53cf

zzcclp force-pushed the wip_upgrade_to_spark2.3.2 branch from 586cf7b to c8b53cf Compare September 28, 2018 14:52

added some comments

8e10273

asfgit closed this in 2081bc8 Sep 30, 2018

[CARBONDATA-2989] Upgrade spark integration version to 2.3.2 #2779

[CARBONDATA-2989] Upgrade spark integration version to 2.3.2 #2779

Uh oh!

Conversation

zzcclp commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzcclp commented Sep 27, 2018

Uh oh!

CarbonDataQA commented Sep 27, 2018

Uh oh!

sujith71955 commented Sep 27, 2018

Uh oh!

CarbonDataQA commented Sep 27, 2018

Uh oh!

CarbonDataQA commented Sep 27, 2018

Uh oh!

chenliang613 commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

ravipesala commented Sep 28, 2018

Uh oh!

zzcclp commented Sep 28, 2018

Uh oh!

zzcclp commented Sep 28, 2018

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

CarbonDataQA commented Sep 28, 2018

Uh oh!

zzcclp commented Sep 28, 2018

Uh oh!

chenliang613 commented Sep 29, 2018

Uh oh!

jackylk commented Sep 29, 2018

Uh oh!

zzcclp commented Sep 29, 2018

Uh oh!

chenliang613 commented Sep 30, 2018

Uh oh!

ravipesala commented Sep 30, 2018

Uh oh!

sujith71955 commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarbonDataQA commented Sep 30, 2018

Uh oh!

CarbonDataQA commented Sep 30, 2018

Uh oh!

CarbonDataQA commented Sep 30, 2018

Uh oh!

zzcclp commented Sep 30, 2018

Uh oh!

zzcclp commented Sep 27, 2018 •

edited

Loading

sujith71955 commented Sep 30, 2018 •

edited

Loading

sujith71955 commented Sep 30, 2018 •

edited

Loading