Fix case for self closing tag inside row #321
Conversation
Add test case for self closing tag inside row
Codecov Report
@@ Coverage Diff @@
## master #321 +/- ##
=====================================
Coverage 0% 0%
=====================================
Files 14 14
Lines 744 753 +9
Branches 54 61 +7
=====================================
- Misses 744 753 +9
Continue to review full report at Codecov.
|
|
@HyukjinKwon I fixed this bug |
|
@srowen can your review this bug fix? |
# Conflicts: # src/test/scala/com/databricks/spark/xml/XmlSuite.scala
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
I think we'd need substantially a different change here, but I'll consider the issue
| override def createRecordReader( | ||
| split: InputSplit, | ||
| context: TaskAttemptContext): RecordReader[LongWritable, Text] = { | ||
| split: InputSplit, |
There was a problem hiding this comment.
Likewise this needs to be reverted
There was a problem hiding this comment.
I can't revert this change, because goal scalacheckstyle will fail on it.
There was a problem hiding this comment.
The line isn't too long if you don't add the deep continuation indent. The code passed style checks before.
| buffer: DataOutputBuffer): Boolean = { | ||
| def checkStartTagBefore = { | ||
| val buf = Seq('<'.toByte) | ||
| val rootTagName = buffer.getData |
There was a problem hiding this comment.
Why define a method here?
I don't think this is efficient enough as it makes a few copies of much of the buffer.
There was a problem hiding this comment.
Because it's used only here.
I don't think this is efficient enough as it makes a few copies of much of the buffer.
maybe, but this code run only in rare situations
code in buffet not so easy to read
There was a problem hiding this comment.
Let me think about it and maybe try a different approach. I think we just need to look for a self-close tag that can only come before any other tag close.
There was a problem hiding this comment.
(BTW here I meant, why define a method at all? it's only used here, as you say)
|
(Wrong PR, this is the one I mean I'll resolve differently now that I merged the other fix) |
|
This should be fixed by #352 |
3 participants
fix #316