Add timestampFormat and dateFormat parsing options #308

jameswinegar · 2018-05-31T04:26:32Z

Fix issues discussed by @HyukjinKwon in #293.
Tests for using a default timestamp of yyyy-MM-dd'T'HH:mm:ss.SSSXXX aren't consistent with timezone awareness in java.sql.timestamp

…ault timestamp of yyyy-MM-dd'T'HH:mm:ss.SSSXXX aren't consistent with timezone awareness in java.sql.timestamp

codecov-io · 2018-05-31T04:31:13Z

Codecov Report

Merging #308 into master will decrease coverage by <.01%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   88.54%   88.53%   -0.01%     
==========================================
  Files          14       14              
  Lines         733      750      +17     
  Branches       93       96       +3     
==========================================
+ Hits          649      664      +15     
- Misses         84       86       +2

Impacted Files	Coverage Δ
...la/com/databricks/spark/xml/util/InferSchema.scala	`87.32% <100%> (+0.18%)`	⬆️
...atabricks/spark/xml/parsers/StaxXmlGenerator.scala	`95.55% <100%> (ø)`	⬆️
...in/scala/com/databricks/spark/xml/XmlOptions.scala	`97.5% <100%> (+0.44%)`	⬆️
...scala/com/databricks/spark/xml/util/TypeCast.scala	`81.31% <91.66%> (-0.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3772b2...629d3db. Read the comment docs.

jameswinegar · 2018-05-31T05:34:06Z

Need to address prior commits to master that used the same files as this PR.

jameswinegar · 2018-05-31T14:12:12Z

src/test/scala/com/databricks/spark/xml/XmlSuite.scala

-
-    assert(df.collect().length == numFiasHouses)
-    assert(df.select().where("_HOUSEID is null").count() == 0)
-  }


jameswinegar · 2018-05-31T14:12:24Z

src/main/scala/com/databricks/spark/xml/util/InferSchema.scala

-    rootValuesMap.foreach {
-      case (f, v) =>
-        nameToDataType += (f -> ArrayBuffer(inferFrom(v, options)))
-    }


jameswinegar · 2018-05-31T14:12:31Z

src/main/scala/com/databricks/spark/xml/util/InferSchema.scala

+            case (f, v) =>
+              nameToDataType += (f -> ArrayBuffer(inferFrom(v, options)))
+          }
+


jameswinegar · 2018-05-31T14:15:05Z

src/test/scala/com/databricks/spark/xml/util/TypeCastSuite.scala

 import java.util.Locale

 import org.scalatest.FunSuite
-


add back for style

jameswinegar · 2018-05-31T14:18:13Z

src/test/scala/com/databricks/spark/xml/XmlSuite.scala

  val numBooksComplicated = 3
  val numTopics = 1
  val numGPS = 2
-  val numFiasHouses = 37


jameswinegar · 2018-05-31T14:18:16Z

src/test/scala/com/databricks/spark/xml/XmlSuite.scala

  val booksRootTag = "books"
  val topicsTag = "Topic"
  val agesTag = "person"
-  val fiasRowTag = "House"


jameswinegar · 2018-05-31T14:18:22Z

src/test/scala/com/databricks/spark/xml/XmlSuite.scala

  val simpleNestedObjects = "src/test/resources/simple-nested-objects.xml"
  val nestedElementWithNameOfParent = "src/test/resources/nested-element-with-name-of-parent.xml"
  val booksMalformedAttributes = "src/test/resources/books-malformed-attributes.xml"
-  val fiasHouse = "src/test/resources/fias_house.xml"


jameswinegar · 2018-05-31T14:18:46Z

src/main/scala/com/databricks/spark/xml/util/InferSchema.scala

    // We need to manually merges the fields having the sames so that
    // This can be inferred as ArrayType.
-    nameToDataType.foreach {
+    nameToDataType.foreach{


jameswinegar · 2018-05-31T14:43:01Z

@HyukjinKwon ready for review I think.

HyukjinKwon · 2018-06-01T01:22:17Z

Hey @jameswinegar, thanks for addressing this one. Will take a look, merge and make a release soon. Thanks again!

jameswinegar · 2018-06-18T18:19:40Z

@HyukjinKwon just wanted to follow up.

HyukjinKwon · 2018-06-21T11:59:06Z

Argh, this weekend ..

jameswinegar · 2018-07-08T04:51:36Z

Following up again

HyukjinKwon

@jameswinegar, would you mind if I ask to remove dateFormat support in this PR? I think in this way we can avoid the type conflict problem in schema inference, and then just go merging it and unblock the release.

Otherwise LGTM if we only go for timestampFormat in this PR for now.

HyukjinKwon · 2018-07-10T05:37:57Z

src/main/scala/com/databricks/spark/xml/XmlOptions.scala

  val DEFAULT_ROOT_TAG = "ROWS"
  val DEFAULT_CHARSET = "UTF-8"
  val DEFAULT_NULL_VALUE = null
+  val DEFAULT_TIMESTAMP_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS"


@jameswinegar, shall we match this to CSV's in Spark? yyyy-MM-dd'T'HH:mm:ss.SSSXXX. IIRC, that complies ISO 8601 and should be good to be consistent with it.

HyukjinKwon · 2018-07-10T05:40:52Z

src/main/scala/com/databricks/spark/xml/util/TypeCast.scala

    }
  }

+  private[xml] def isDate(value: String, dateFormatter: SimpleDateFormat = null) = {


@jameswinegar, seems there looks no case where dateFormatter should be omitted. I think we can just isDate(value: String, dateFormatter: SimpleDateFormat)

HyukjinKwon · 2018-07-10T05:47:46Z

src/main/scala/com/databricks/spark/xml/util/InferSchema.scala

      FloatType,
      DoubleType,
      TimestampType,
+      DateType,


Hm .. this actually needs a more fundamental fix. We should not state the precedence in this order (date is wider then timestamp). I am pretty sure this could lead to ending up with date during resolving the type conflicts between date and timestamp.

For example,

"1999/01/11" -> date "2017/05/15T14:58:19Z" -> timestamp

then it becomes date time which I guess will truncate the time part in the timestamp.

HyukjinKwon · 2018-07-10T05:49:39Z

#316 actually a release blocker too. Let me revert this and make a release if no fix looks going to be landed soon.

jameswinegar · 2018-09-15T03:29:14Z

@HyukjinKwon can we put eyes on this again? see #321

BioQwer · 2018-11-30T10:05:49Z

@jameswinegar #321 is ready

HyukjinKwon · 2018-12-22T10:29:36Z

That's fixed now. Can you update this PR @jameswinegar?

jameswinegar · 2018-12-22T13:52:43Z

I'll try and get to it sometime this week.

…

On Sat, Dec 22, 2018, 4:29 AM Hyukjin Kwon ***@***.*** wrote: That's fixed now. Can you update this PR @jameswinegar <https://github.com/jameswinegar>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIC0yX8tJZqFaX8MXoO7Vi7qzPG8sr2Kks5u7gmSgaJpZM4UUZqy> .

srowen · 2019-10-28T00:44:38Z

You are welcome to reopen with new commits

Fix issues discussed by @HyukjinKwon in PR 293. Tests for using a def…

609f2dd

…ault timestamp of yyyy-MM-dd'T'HH:mm:ss.SSSXXX aren't consistent with timezone awareness in java.sql.timestamp

jameswinegar closed this May 31, 2018

jameswinegar commented May 31, 2018

View reviewed changes

Fix issues relating to code changes in prior commits to master.

629d3db

jameswinegar reopened this May 31, 2018

HyukjinKwon reviewed Jul 10, 2018

View reviewed changes

HyukjinKwon mentioned this pull request Dec 22, 2018

Add timestampFormat and dateFormat parsing options #293

Closed

srowen closed this Oct 28, 2019

Add timestampFormat and dateFormat parsing options #308

Add timestampFormat and dateFormat parsing options #308

Uh oh!

Conversation

jameswinegar commented May 31, 2018

Uh oh!

codecov-io commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jameswinegar commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameswinegar commented May 31, 2018

Uh oh!

HyukjinKwon commented Jun 1, 2018

Uh oh!

jameswinegar commented Jun 18, 2018

Uh oh!

HyukjinKwon commented Jun 21, 2018

Uh oh!

jameswinegar commented Jul 8, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 10, 2018

Uh oh!

jameswinegar commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BioQwer commented Nov 30, 2018

Uh oh!

HyukjinKwon commented Dec 22, 2018

Uh oh!

jameswinegar commented Dec 22, 2018 via email

Uh oh!

srowen commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented May 31, 2018 •

edited

Loading

jameswinegar commented May 31, 2018 •

edited

Loading

jameswinegar commented Sep 15, 2018 •

edited

Loading