Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CARBONDATA-1669] Clean up code in CarbonDataRDDFactory #1467

Closed
wants to merge 3 commits into from
Closed

[CARBONDATA-1669] Clean up code in CarbonDataRDDFactory #1467

wants to merge 3 commits into from

Conversation

jackylk
Copy link
Contributor

@jackylk jackylk commented Nov 5, 2017

Inside CarbonDataRDDFactory.loadCarbonData, there are many function defined inside function, makes the loading logic very hard to read
This PR improves its readability

  • Any interfaces changed?
    No

  • Any backward compatibility impacted?
    No

  • Document update required?
    No

  • Testing done
    No new testcase is required

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
    NA

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/811/

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1444/

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/818/

@jackylk
Copy link
Contributor Author

jackylk commented Nov 6, 2017

retest this please

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/821/

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1450/

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1451/

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1454/

@ravipesala
Copy link
Contributor

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1472/

@jackylk
Copy link
Contributor Author

jackylk commented Nov 7, 2017

retest this please

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/840/

QiangCai
QiangCai reviewed Nov 7, 2017
View reviewed changes
Copy link
Contributor

@QiangCai QiangCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to keep previous code style.

integration/spark-common/src/main/scala/org/apache/carbondata/api/CarbonStore.scala Outdated
@@ -79,16 +82,45 @@ object CarbonStore {
dbName: String,
tableName: String,
storePath: String,
carbonTable: CarbonTable, forceTableClean: Boolean): Unit = {
carbonTable: CarbonTable, forceTableClean: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one parameter one row

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

integration/spark-common/src/main/scala/org/apache/carbondata/api/CarbonStore.scala Outdated
}
Seq.empty
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why separate it to two try-catch block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

integration/spark-common/src/main/scala/org/apache/carbondata/api/CarbonStore.scala Outdated
@@ -79,16 +82,45 @@ object CarbonStore {
dbName: String,
tableName: String,
storePath: String,
carbonTable: CarbonTable, forceTableClean: Boolean): Unit = {
carbonTable: CarbonTable, forceTableClean: Boolean
): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to last row

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

...in/scala/org/apache/spark/sql/execution/command/management/AlterTableCompactionCommand.scala Outdated
carbonLoadModel
.setSegmentUpdateStatusManager(alterTableModel.segmentUpdateStatusManager.get)

carbonLoadModel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to :
carbonLoadModel.setLoadMetadataDetails(
alterTableModel.segmentUpdateStatusManager.get.getLoadMetadataDetails.toList.asJava)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1473/

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/842/

@ravipesala
Copy link
Contributor

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1475/

chenliang613
chenliang613 reviewed Nov 7, 2017
View reviewed changes
...gration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/DataManagementFunc.scala
@@ -49,105 +44,6 @@ object DataManagementFunc {

private val LOGGER = LogServiceFactory.getLogService(this.getClass.getCanonicalName)

def deleteLoadByDate(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirm : this method(deleteLoadByDate) is for spark 1.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one use this function

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/846/

@ravipesala
Copy link
Contributor

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1479/

@QiangCai
Copy link
Contributor

QiangCai commented Nov 7, 2017

LGTM

@asfgit asfgit closed this in 0578ba0 Nov 7, 2017
jatin9896 pushed a commit to jatin9896/incubator-carbondata that referenced this pull request Jan 5, 2018
@jackylk @jatin9896
[CARBONDATA-1669] Clean up code in CarbonDataRDDFactory
20367f8 
This closes apache#1467
anubhav100 pushed a commit to anubhav100/incubator-carbondata that referenced this pull request Jun 22, 2018
@jackylk @anubhav100
[CARBONDATA-1669] Clean up code in CarbonDataRDDFactory
20cd578 
This closes apache#1467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenliang613 chenliang613 chenliang613 left review comments

@QiangCai QiangCai QiangCai left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jackylk @CarbonDataQA @ravipesala @QiangCai @chenliang613