You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "steven-aerts (via GitHub)" <gi...@apache.org> on 2023/04/27 09:45:06 UTC
[GitHub] [avro] steven-aerts opened a new pull request, #2203: AVRO-3748 [java] fix SeekableInputStream.skip
steven-aerts opened a new pull request, #2203:
URL: https://github.com/apache/avro/pull/2203
The implementation of SeekableInputStream.skip had a longstantind issue that it did a seek to a wrong position.
This issue is rather difficult to hit in avro as it is only used in corner cases of for example the FastReader.
## What is the purpose of the change
* Fixes AVRO-3748 a longstanding issue in SeekableInputStream.skip
## Verifying this change
This change added a test to verify the new behavior
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #2203: AVRO-3748 [java] fix SeekableInputStream.skip
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #2203:
URL: https://github.com/apache/avro/pull/2203#discussion_r1187149460
##########
lang/java/avro/src/test/java/org/apache/avro/file/TestSeekableInputStream.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.avro.file;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.nio.file.Path;
+
+public class TestSeekableInputStream {
+ @Test
+ public void testSkip(@TempDir Path tempDir) throws IOException {
+ File f = tempDir.resolve("testSkip.avro").toFile();
+ try (FileWriter w = new FileWriter(f)) {
+ w.write("someContent");
+ }
+ try(DataFileReader.SeekableInputStream stream =
+ new DataFileReader.SeekableInputStream(new SeekableFileInput(f))
+ ) {
+ for (int i = 0; i < stream.length(); i++) {
Review Comment:
## Comparison of narrow type with wide type in loop condition
Comparison between [expression](1) of type int and [expression](2) of wider type long.
[Show more details](https://github.com/apache/avro/security/code-scanning/2986)
##########
lang/java/avro/src/test/java/org/apache/avro/file/TestSeekableInputStream.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.avro.file;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.nio.file.Path;
+
+public class TestSeekableInputStream {
+ @Test
+ public void testSkip(@TempDir Path tempDir) throws IOException {
+ File f = tempDir.resolve("testSkip.avro").toFile();
+ try (FileWriter w = new FileWriter(f)) {
+ w.write("someContent");
+ }
+ try(DataFileReader.SeekableInputStream stream =
+ new DataFileReader.SeekableInputStream(new SeekableFileInput(f))
Review Comment:
## Potential input resource leak
This SeekableFileInput is not always closed on method exit.
[Show more details](https://github.com/apache/avro/security/code-scanning/2985)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on a diff in pull request #2203: AVRO-3748 [java] fix SeekableInputStream.skip
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2203:
URL: https://github.com/apache/avro/pull/2203#discussion_r1182480339
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader.java:
##########
@@ -312,10 +312,10 @@ public long skip(long skip) throws IOException {
long length = in.length();
long remaining = length - position;
if (remaining > skip) {
- in.seek(skip);
+ in.seek(position + skip);
return in.tell() - position;
Review Comment:
And in same time, either `return skip; ... else { ... return length - position; }` either put `return in.tell() - position;` at the end of function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] steven-aerts commented on a diff in pull request #2203: AVRO-3748 [java] fix SeekableInputStream.skip
Posted by "steven-aerts (via GitHub)" <gi...@apache.org>.
steven-aerts commented on code in PR #2203:
URL: https://github.com/apache/avro/pull/2203#discussion_r1187178181
##########
lang/java/avro/src/test/java/org/apache/avro/file/TestSeekableInputStream.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.avro.file;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.nio.file.Path;
+
+public class TestSeekableInputStream {
+ @Test
+ public void testSkip(@TempDir Path tempDir) throws IOException {
+ File f = tempDir.resolve("testSkip.avro").toFile();
+ try (FileWriter w = new FileWriter(f)) {
+ w.write("someContent");
+ }
+ try(DataFileReader.SeekableInputStream stream =
+ new DataFileReader.SeekableInputStream(new SeekableFileInput(f))
Review Comment:
Done
##########
lang/java/avro/src/test/java/org/apache/avro/file/TestSeekableInputStream.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.avro.file;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.nio.file.Path;
+
+public class TestSeekableInputStream {
+ @Test
+ public void testSkip(@TempDir Path tempDir) throws IOException {
+ File f = tempDir.resolve("testSkip.avro").toFile();
+ try (FileWriter w = new FileWriter(f)) {
+ w.write("someContent");
+ }
+ try(DataFileReader.SeekableInputStream stream =
+ new DataFileReader.SeekableInputStream(new SeekableFileInput(f))
+ ) {
+ for (int i = 0; i < stream.length(); i++) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec merged pull request #2203: AVRO-3748 [java] fix SeekableInputStream.skip
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec merged PR #2203:
URL: https://github.com/apache/avro/pull/2203
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org