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-4109] Improve carbondata coverage for presto-integration code #4074

Closed
wants to merge 1 commit into from
Closed

[CARBONDATA-4109] Improve carbondata coverage for presto-integration code #4074

wants to merge 1 commit into from

Conversation

akkio-97
Copy link
Contributor

@akkio-97 akkio-97 commented Jan 11, 2021 •
edited

Why is this PR needed?

Few scenarios had missing coverage in presto-integration code. This PR aims to improve it by considering all such scenarios.

Dead code- ObjectStreamReader.java was created with an aim to query complex types. Instead ComplexTypeStreamReader was created. Making ObjectStreamreader obsolete.

What changes were proposed in this PR?

  • Test cases added for scenarios that were not covered earlier in presto-integration code
  • Removed dead code.

Does this PR introduce any user interface change?

  • No

Is any new testcase added?

  • Yes

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

@QiangCai
Copy link
Contributor

please change the PR title

@akkio-97 akkio-97 changed the title [WIP] [WIP] Improved Presto coverage and removed dead code Jan 13, 2021
@ajantha-bhat
Copy link
Member

ajantha-bhat commented Jan 19, 2021 •
edited

please change the PR title

@akkio-97 : Yes, it is not improving presto coverage. It is improving carbondata coverage for presto integration code.

@akkio-97 akkio-97 changed the title [WIP] Improved Presto coverage and removed dead code [CARBONDATA-4109] Improve carbondata coverage for presto-integration code Jan 19, 2021
@akkio-97
Copy link
Contributor Author

please change the PR title

@akkio-97 : Yes, it is not improving presto coverage. It is improving carbondata coverage for presto integration code.

changed the title

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3577/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5337/

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5355/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3595/

@akkio-97
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5356/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3596/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5363/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3603/

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

ajantha-bhat
ajantha-bhat reviewed Jan 29, 2021
View reviewed changes
.../org/apache/carbondata/integration/spark/testsuite/dataload/SparkStoreCreatorForPresto.scala Outdated
// page datatype - long, adaptive integral

// set timezone back to default
TimeZone.setDefault(TimeZone.getTimeZone("Asia/Shanghai"))
Copy link
Member

Choose a reason for hiding this comment

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

Can you get a getdefault() value in a variable before setting in line 435 and set it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done

ajantha-bhat
ajantha-bhat reviewed Jan 29, 2021
View reviewed changes
...ala/org/apache/carbondata/spark/testsuite/dataload/TestLoadDataWithDiffTimestampFormat.scala Outdated
@@ -354,6 +355,7 @@ class TestLoadDataWithDiffTimestampFormat extends QueryTest with BeforeAndAfterA

test("test load, update data with setlenient session level property for daylight " +
"saving time from different timezone") {
TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles"))
Copy link
Member

Choose a reason for hiding this comment

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

If the above comment works, you can remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

ajantha-bhat
ajantha-bhat reviewed Jan 29, 2021
View reviewed changes
.../src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestUsingSparkStore.scala Outdated
@@ -20,14 +20,15 @@ package org.apache.carbondata.presto.integrationtest
import java.io.{File}
import java.util

import io.prestosql.jdbc.PrestoArray
Copy link
Member

Choose a reason for hiding this comment

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

I think presto db compile will fail after this. can you handle validation in the util method like PrestoTestUtil#validateArrayOfPrimitiveTypeData

Copy link
Member

Choose a reason for hiding this comment

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

Also please verify presto db compile once to catch if any other things broke compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ajantha-bhat
Copy link
Member

@akkio-97 : Please rebase the PR

@ajantha-bhat
Copy link
Member

LGTM

@CarbonDataQA2
Copy link

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

@CarbonDataQA2
Copy link

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

@asfgit asfgit closed this in 46a46a0 Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajantha-bhat ajantha-bhat ajantha-bhat 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
4 participants
@akkio-97 @CarbonDataQA2 @QiangCai @ajantha-bhat