Skip to content

Conversation

@eyalfa
Copy link

@eyalfa eyalfa commented Jul 29, 2016

What changes were proposed in this pull request?

a failing test case + fix to SPARK-16791 (https://issues.apache.org/jira/browse/SPARK-16791)

How was this patch tested?

added a failing test case to CastSuit, then fixed the Cast code and rerun the entire CastSuit

@eyalfa
Copy link
Author

eyalfa commented Jul 29, 2016

@ueshin , you seem to be the original committer of this code. can you please have a look?


private[this] def cast(from: DataType, to: DataType): Any => Any = to match {
case dt if dt == child.dataType => identity[Any]
case dt if dt == from => identity[Any]
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove extra whitespace before from.

@ueshin
Copy link
Member

ueshin commented Jul 29, 2016

Good catch!
I have some comments, please check them.
/cc @marmbrus @cloud-fan

@cloud-fan
Copy link
Contributor

Good catch!

LGTM except the style comment given by @ueshin . And please also use full word for variable name, e.g. originalSchema, targetSchema

}

test("cast struct with a timestamp field") {
val originalSchema = StructType(
Copy link
Contributor

Choose a reason for hiding this comment

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

we usually use new StructType().add("name", TimestampType, nullable = false)

}

test("cast struct with a timestamp field") {
val originalSchema = new StructType().add( "tsField", TimestampType, nullable = false )
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the space in add( xxx )

@cloud-fan
Copy link
Contributor

ok to test


val inp = Literal.create(InternalRow(0L), originalSchema)
val expected = InternalRow(0L)
checkEvaluation(cast(inp, targetSchema), expected )
Copy link
Contributor

Choose a reason for hiding this comment

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

and remove the extra space here too

@SparkQA
Copy link

SparkQA commented Jul 31, 2016

Test build #63052 has finished for PR 14400 at commit 6e7f69f.

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

@eyalfa
Copy link
Author

eyalfa commented Aug 1, 2016

@cloud-fan, any update on this?

@cloud-fan
Copy link
Contributor

#14400 (comment)

@eyalfa
Copy link
Author

eyalfa commented Aug 1, 2016

@cloud-fan, just for notice for the next patches: is there a way to run mvn in a mode that fails on style issues? it could save a lot of round-trips if it existed. (can be sbt just as well)

@cloud-fan
Copy link
Contributor

we have an individual style check script: ./dev/scalastyle

@SparkQA
Copy link

SparkQA commented Aug 1, 2016

Test build #63076 has finished for PR 14400 at commit e0549a9.

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

asfgit pushed a commit that referenced this pull request Aug 1, 2016
## What changes were proposed in this pull request?
a failing test case + fix to SPARK-16791 (https://issues.apache.org/jira/browse/SPARK-16791)

## How was this patch tested?
added a failing test case to CastSuit, then fixed the Cast code and rerun the entire CastSuit

Author: eyal farago <eyal farago>
Author: Eyal Farago <[email protected]>

Closes #14400 from eyalfa/SPARK-16791_cast_struct_with_timestamp_field_fails.

(cherry picked from commit 338a98d)
Signed-off-by: Wenchen Fan <[email protected]>
@asfgit asfgit closed this in 338a98d Aug 1, 2016
@cloud-fan
Copy link
Contributor

thanks, merging to master and 2.0!

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.

4 participants