Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Feb 3, 2017

What changes were proposed in this pull request?

This PR proposes to

The usage was removed while refactoring/fixing in several JIRAs such as SPARK-16714, SPARK-16735 and SPARK-16646

How was this patch tested?

Existing tests.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Feb 3, 2017

cc @gatorsmile, could you take a look please?

@SparkQA
Copy link

SparkQA commented Feb 3, 2017

Test build #72308 has finished for PR 16786 at commit c1a2bbb.

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

@gatorsmile
Copy link
Member

Could you rename findTightestCommonTypeOfTwo to findTightestCommonType?

@gatorsmile
Copy link
Member

Also clean up the comments?

Rename findTightestCommonTypeOfTwo to findTightestCommonType with some comments cleanup

Clean up more
@HyukjinKwon
Copy link
Member Author

Thanks, I just updated and rebased.

*/
def compatibleType(t1: DataType, t2: DataType): DataType = {
TypeCoercion.findTightestCommonTypeOfTwo(t1, t2).getOrElse {
TypeCoercion.findTightestCommonType(t1, t2).getOrElse {
Copy link
Member

Choose a reason for hiding this comment

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

In CSVInferSchema.scala, it sounds like we are also calling the same function. Could you check whether we can directly call it, instead of duplicating the code? (I did not read it carefully)

https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala#L172-L217

Copy link
Member Author

@HyukjinKwon HyukjinKwon Feb 4, 2017

Choose a reason for hiding this comment

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

Oh, yea, it appearntly possible for now. But, can I maybe do this in another PR if you think it is fine? It seems possible but I would like to avoid to fix it here simply because of a worry to resolve a conflict in the PR refacorting this code path #16680 and it seems not directly related to the JIRA.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, no problem. Please submit a separate PR for it. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for bearing with me.

@SparkQA
Copy link

SparkQA commented Feb 4, 2017

Test build #72332 has finished for PR 16786 at commit 66a09ed.

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

@HyukjinKwon
Copy link
Member Author

retest this please

@gatorsmile
Copy link
Member

LGTM pending test

@SparkQA
Copy link

SparkQA commented Feb 4, 2017

Test build #72346 has finished for PR 16786 at commit 66a09ed.

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

@gatorsmile
Copy link
Member

Thanks, merging to master!

@asfgit asfgit closed this in 2f3c20b Feb 4, 2017
@HyukjinKwon
Copy link
Member Author

Thank you @gatorsmile.

cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
## What changes were proposed in this pull request?

This PR proposes to

- remove unused `findTightestCommonType` in `TypeCoercion` as suggested in apache#16777 (comment)
- rename `findTightestCommonTypeOfTwo ` to `findTightestCommonType`.
- fix comments accordingly

The usage was removed while refactoring/fixing in several JIRAs such as SPARK-16714, SPARK-16735 and SPARK-16646

## How was this patch tested?

Existing tests.

Author: hyukjinkwon <gurwls223@gmail.com>

Closes apache#16786 from HyukjinKwon/SPARK-19446.
@HyukjinKwon HyukjinKwon deleted the SPARK-19446 branch January 2, 2018 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants