Skip to content

[SPARK-27182][SQL] Move the conflict source code of the sql/core module to sql/core/v1.2.1 #24119

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
Closed
wants to merge 6 commits into from
Closed

[SPARK-27182][SQL] Move the conflict source code of the sql/core module to sql/core/v1.2.1 #24119

wants to merge 6 commits into from

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Mar 17, 2019 •
edited
Loading

What changes were proposed in this pull request?

To make #23788 easy to review. This PR moves OrcColumnVector.java, OrcShimUtils.scala, OrcFilters.scala and OrcFilterSuite.scala to sql/core/v1.2.1 and copies it to sql/core/v2.3.4.

How was this patch tested?

manual tests

diff -urNa sql/core/v1.2.1 sql/core/v2.3.4

@wangyum wangyum mentioned this pull request Mar 17, 2019
Closed
@SparkQA
Copy link

SparkQA commented Mar 17, 2019

Test build #103582 has finished for PR 24119 at commit 5095561.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Mar 17, 2019

What are the differences between the copies of these files? is the idea that there is no difference here but your other PR will change the 2.3.4 copy?

@wangyum
Copy link
Member Author

wangyum commented Mar 18, 2019

Yes. I will change it in other PR. Make other PR easy to review.

@gatorsmile
Copy link
Member

gatorsmile commented Mar 18, 2019

cc @liancheng

I am wondering if we can reduce the code duplicate?

@wangyum
Copy link
Member Author

wangyum commented Mar 19, 2019

Yes. This time revert OrcDeserializer and OrcSerializer and moved checkNoFilterPredicate to the super class(OrcTest).

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103659 has finished for PR 24119 at commit 511ae8d.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Mar 19, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103664 has finished for PR 24119 at commit 511ae8d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103709 has finished for PR 24119 at commit 020d7e7.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Mar 20, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103719 has finished for PR 24119 at commit 020d7e7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen reviewed Mar 20, 2019
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

If this is just a code move/copy to set up another PR, and there's no behavior change, and there is no more duplication reduce, looks good to me.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 21, 2019

So do we need #24166 before this one?

@wangyum wangyum mentioned this pull request Mar 22, 2019
Closed
@wangyum
Copy link
Member Author

wangyum commented Mar 22, 2019

Yes. Thanks @HyukjinKwon

@srowen
Copy link
Member

srowen commented Mar 26, 2019

@wangyum #24166 is merged now

wangyum added 2 commits March 26, 2019 09:43
@wangyum
Merge remote-tracking branch 'upstream/master' into SPARK-27182
a28ee46 
# Conflicts:
#	sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcDeserializer.scala
#	sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcSerializer.scala
@wangyum
Merge master
11bc982
@wangyum
Copy link
Member Author

wangyum commented Mar 26, 2019

Thank you @srowen

@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103935 has finished for PR 24119 at commit 11bc982.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Mar 26, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103951 has finished for PR 24119 at commit 11bc982.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member

maropu commented Mar 26, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103960 has finished for PR 24119 at commit 11bc982.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

So this is again basically moving code and making some copies of some code, so that those copies can be modified in a next PR? if so seems OK.

@liancheng
Copy link
Contributor

liancheng commented Mar 27, 2019

Hey @wangyum, sorry for the long delay. IIUC, this PR is basically a subset of #23788. Once this one is merged, we can simplify #23788 by removing changes made in this PR, right? In that case, this one LGTM.

@wangyum
Copy link
Member Author

wangyum commented Mar 27, 2019

@liancheng Yes. It's a subset of #23788.

HyukjinKwon
HyukjinKwon approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

Looks fine to me too

gatorsmile
gatorsmile reviewed Mar 27, 2019
View reviewed changes
sql/core/v2.3.4/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala
* builder methods mentioned above can only be found in test code, where all tested filters are
* known to be convertible.
*/
private[sql] object OrcFilters extends OrcFiltersBase {
Copy link
Member

@gatorsmile gatorsmile Mar 27, 2019

Choose a reason for hiding this comment

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

All the functions in this object are different between v2.3.4 and v1.2?

Copy link
Member

@gatorsmile gatorsmile Mar 27, 2019

Choose a reason for hiding this comment

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

Talk with @wangyum offline. All these functions need a change in v2.3.4.

@gatorsmile
Copy link
Member

gatorsmile commented Mar 27, 2019

I am merging this to master. @wangyum Please submit your follow-up PR for making the actual changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

@srowen srowen srowen approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@wangyum @SparkQA @srowen @gatorsmile @HyukjinKwon @maropu @liancheng