Skip to content

[SPARK-25600][SQL][MINOR] Make use of TypeCoercion.findTightestCommonType while inferring CSV schema. - #22619

Closed
dilipbiswal wants to merge 3 commits into
apache:masterfrom
dilipbiswal:csv_minor
Closed

[SPARK-25600][SQL][MINOR] Make use of TypeCoercion.findTightestCommonType while inferring CSV schema.#22619
dilipbiswal wants to merge 3 commits into
apache:masterfrom
dilipbiswal:csv_minor

Conversation

@dilipbiswal

@dilipbiswaldilipbiswal commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Current the CSV's infer schema code inlines TypeCoercion.findTightestCommonType. This is a minor refactor to make use of the common type coercion code when applicable. This way we can take advantage of any improvement to the base method.

Thanks to @MaxGekk for finding this while reviewing another PR.

How was this patch tested?

This is a minor refactor. Existing tests are used to verify the change.

@SparkQA

SparkQA commented Oct 3, 2018

Copy link
Copy Markdown

Test build #96881 has finished for PR 22619 at commit d4e0bdb.

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

@dilipbiswal

dilipbiswal commented Oct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

cc @HyukjinKwon@MaxGekk

@gatorsmile

gatorsmile commented Oct 3, 2018

Copy link
Copy Markdown
Member

Any behavior change? Test cases?

@ueshin

ueshin commented Oct 3, 2018

Copy link
Copy Markdown
Member

Maybe this is related to #22448.

@dilipbiswal

dilipbiswal commented Oct 3, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@gatorsmile There should not be any behaviour change. I was thinking that existing test cases should suffice. Basically we used to duplicate the code of TypeCoercion.findTightestCommonType in here. Here i am just reusing the common function. This is tested in CSVInferSchemaSuite

@dilipbiswal

dilipbiswal commented Oct 3, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@ueshin

Maybe this is related to #22448.

Yeah.. Actually @MaxGekk had pointed me to the presence of duplicate code in one of his comment. I was trying to address it in here.

HyukjinKwon
HyukjinKwon reviewed Oct 3, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala Outdated
} else {
Some(DecimalType(range + scale, scale))
}
case (_, _) => None

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

case _ => None

@dilipbiswaldilipbiswalOct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@HyukjinKwon Thanks. Will change.

HyukjinKwon
HyukjinKwon reviewed Oct 3, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala Outdated
Some(DecimalType(range + scale, scale))
def compatibleType(t1: DataType, t2: DataType): Option[DataType] = {
TypeCoercion.findTightestCommonType(t1, t2).orElse {
(t1, t2) match {

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we leave this out as a private val like the previous and leave a comment that this pattern matching is CSV specific? That will reduce the diff and makes the review easier.

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW, let's keep the comments in the original place.

@dilipbiswaldilipbiswalOct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@HyukjinKwon Did you have any preference or suggestion on the name of the val ? findCommonTypeExtended ?

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not sure. maybe just findCompatibleTypeForCSV

@HyukjinKwon

HyukjinKwon commented Oct 3, 2018

Copy link
Copy Markdown
Member

Looks okay - I checked a case one by one but it needs another look.

@HyukjinKwon

HyukjinKwon commented Oct 3, 2018

Copy link
Copy Markdown
Member

Let's just file a JIRA @dilipbiswal BTW.

@dilipbiswal

dilipbiswal commented Oct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

@HyukjinKwon Okay.

viirya
viirya reviewed Oct 3, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala
findTightestCommonType(t1, DecimalType.forType(t2))

// Double support larger range than fixed decimal, DecimalType.Maximum should be enough
// in most case, also have better precision.

@viiryaviiryaOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments here are ignored in the change. Shall we keep them?

@dilipbiswaldilipbiswalOct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@viirya Yeah.. we should keep.. sorry.. got dropped inadvertently.

viirya
viirya reviewed Oct 3, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala Outdated
Some(DoubleType)
} else {
Some(DecimalType(range + scale, scale))
def compatibleType(t1: DataType, t2: DataType): Option[DataType] = {

@viiryaviiryaOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: findCompatibleType?

@dilipbiswaldilipbiswalOct 3, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@viirya i kept the same name used in JsonInferSchema. Change that as well ? Or only change this ?

@viiryaviiryaOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

compatibleType is also fine if it is consistent with JsonInferSchema.

@dilipbiswaldilipbiswal changed the title [SQL][MINOR] Make use of TypeCoercion.findTightestCommonType while inferring CSV schema.[SPARK-25600][SQL][MINOR] Make use of TypeCoercion.findTightestCommonType while inferring CSV schema.Oct 3, 2018
HyukjinKwon
HyukjinKwon approved these changes Oct 3, 2018
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala Outdated

case _ => None
}

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's get rid of new lines changes.

Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala Outdated
* is compatible with both input data types.
*/
private def compatibleType(t1: DataType, t2: DataType): Option[DataType] = {
TypeCoercion.findTightestCommonType(t1, t2).orElse (findCompatibleTypeForCSV(t1, t2))

@HyukjinKwonHyukjinKwonOct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: e ( -> e(

@SparkQA

SparkQA commented Oct 3, 2018

Copy link
Copy Markdown

Test build #96890 has finished for PR 22619 at commit ad69a1b.

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

@SparkQA

SparkQA commented Oct 3, 2018

Copy link
Copy Markdown

Test build #96897 has finished for PR 22619 at commit 9e656a8.

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

@dilipbiswal

dilipbiswal commented Oct 5, 2018

Copy link
Copy Markdown
ContributorAuthor

@HyukjinKwon Does this look okay now ?

@HyukjinKwon

HyukjinKwon commented Oct 5, 2018

Copy link
Copy Markdown
Member

Yup. Let me leave this open few more days in case.

@dilipbiswal

dilipbiswal commented Oct 5, 2018

Copy link
Copy Markdown
ContributorAuthor

@HyukjinKwon Sure :-)

@HyukjinKwon

HyukjinKwon commented Oct 6, 2018

Copy link
Copy Markdown
Member

Merged to master.

@asfgitasfgit closed this in f2f4e7aOct 6, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@dilipbiswalJackey Lee
[SPARK-25600][SQL][MINOR] Make use of TypeCoercion.findTightestCommon…
685d8de
…Type while inferring CSV schema.
## What changes were proposed in this pull request?
Current the CSV's infer schema code inlines `TypeCoercion.findTightestCommonType`. This is a minor refactor to make use of the common type coercion code when applicable. This way we can take advantage of any improvement to the base method.
Thanks to MaxGekk for finding this while reviewing another PR.
## How was this patch tested?
This is a minor refactor. Existing tests are used to verify the change.
Closesapache#22619 from dilipbiswal/csv_minor.
Authored-by: Dilip Biswal <dbiswal@us.ibm.com>
Signed-off-by: hyukjinkwon <gurwls223@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viiryaviiryaviirya left review comments
@HyukjinKwonHyukjinKwonHyukjinKwon 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.

6 participants

@dilipbiswal@SparkQA@gatorsmile@ueshin@HyukjinKwon@viirya