You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by jinossy <gi...@git.apache.org> on 2014/12/05 11:21:27 UTC
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
GitHub user jinossy opened a pull request:
https://github.com/apache/tajo/pull/289
TAJO-1235: ByteBufLineReader can not read text line with CRLF
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinossy/tajo TAJO-1235
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/289.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #289
----
commit 3dbf0775b65823d544d09262f11e4544576b06b6
Author: jhkim <jh...@apache.org>
Date: 2014-12-05T10:19:14Z
TAJO-1235: ByteBufLineReader can not read text line with CRLF
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/289#discussion_r21501490
--- Diff: tajo-storage/src/main/java/org/apache/tajo/storage/text/ByteBufLineReader.java ---
@@ -77,23 +74,25 @@ private void fillBuffer() throws IOException {
int tailBytes = 0;
if (this.readBytes > 0) {
+ //startIndex = 0, readIndex = tailBytes length, writable = (buffer capacity - tailBytes)
this.buffer.markReaderIndex();
- this.buffer.discardSomeReadBytes(); // compact the buffer
+ this.buffer.discardReadBytes(); // compact the buffer
tailBytes = this.buffer.writerIndex();
if (!this.buffer.isWritable()) {
// a line bytes is large than the buffer
- BufferPool.ensureWritable(buffer, bufferSize);
+ BufferPool.ensureWritable(buffer, bufferSize * 2);
this.bufferSize = buffer.capacity();
}
+ this.startIndex = 0;
}
boolean release = true;
try {
int readBytes = tailBytes;
for (; ; ) {
- int localReadBytes = buffer.writeBytes(channel, bufferSize - readBytes);
+ int localReadBytes = buffer.writeBytes(channel, this.bufferSize - readBytes);
if (localReadBytes < 0) {
- if (tailBytes == readBytes) {
+ if (buffer.isWritable()) {
--- End diff --
It seems that we can ensure EOF if there is no read bytes and there are write buffer. But, it may be hard for code readers to imagine this logic. Could you add some comments about this logic?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/289#discussion_r21501546
--- Diff: tajo-storage/src/main/java/org/apache/tajo/storage/text/ByteBufLineReader.java ---
@@ -147,19 +155,19 @@ public ByteBuf readLineBuf(AtomicInteger reads) throws IOException {
//does not appeared terminating newline
buffer.readerIndex(buffer.writerIndex()); // set to end buffer
if(eof){
- readBytes = buffer.readerIndex() - startIndex;
- newlineLength = 0;
+ readBytes += buffer.readerIndex() - startIndex;
break loop;
}
} else {
buffer.readerIndex(endIndex + 1);
- readBytes = buffer.readerIndex() - startIndex;
+ readBytes += (buffer.readerIndex() - startIndex);
if (processor.isPrevCharCR() && buffer.isReadable()
--- End diff --
Could you add comments about this condition?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/289#issuecomment-66219912
The patch looks good to me. It's a nice fix. It would be great if you add some comments about if-condition branches.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/289
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/289#issuecomment-66225661
Thank you for the review. I've left some comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/289#issuecomment-66256498
+1
The patch looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/289#issuecomment-66034084
Could you trigger unit tests?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1235: ByteBufLineReader can not read text ...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/289#discussion_r21501517
--- Diff: tajo-storage/src/main/java/org/apache/tajo/storage/text/ByteBufLineReader.java ---
@@ -120,24 +118,34 @@ private void fillBuffer() throws IOException {
* Read a line terminated by one of CR, LF, or CRLF.
*/
public ByteBuf readLineBuf(AtomicInteger reads) throws IOException {
- if(eof) return null;
-
- int startIndex = buffer.readerIndex();
- int readBytes;
+ int readBytes = 0;
+ int newlineLength = 0; //length of terminating newline
int readable;
- int newlineLength; //length of terminating newline
+
+ this.startIndex = buffer.readerIndex();
loop:
while (true) {
readable = buffer.readableBytes();
if (readable <= 0) {
- buffer.readerIndex(startIndex);
+ buffer.readerIndex(this.startIndex);
fillBuffer(); //compact and fill buffer
- if (!buffer.isReadable()) {
+ if (!buffer.isReadable() && buffer.writerIndex() == 0) {
--- End diff --
Could you add comments about this case?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---