steveloughran commented on code in PR #7291:
URL: https://github.com/apache/hadoop/pull/7291#discussion_r1918456038
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestTextCommand.java:
##########
@@ -67,39 +75,194 @@ public void testDisplayForAvroFiles() throws Exception {
assertEquals(expectedOutput, output);
}
+ @Test
+ public void testEmptyAvroFile() throws Exception {
Review Comment:
I'm going to suggest something different: add an abstract contract test for
this command and implement it in file:, hdfs, s3a and azure -then in gcs once
that is in.
That way we can see how well/badly those stores cope with this
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestTextCommand.java:
##########
@@ -67,39 +75,194 @@ public void testDisplayForAvroFiles() throws Exception {
assertEquals(expectedOutput, output);
}
+ @Test
+ public void testEmptyAvroFile() throws Exception {
+ String output = readUsingTextCommand(AVRO_FILENAME,
+ generateEmptyAvroBinaryData());
+ assertTrue(output.isEmpty());
+ }
+
+ @Test(expected = NullPointerException.class)
+ public void testAvroFileInputStreamNullBuffer() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(null, 0, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamNegativePosition() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], -1, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamTooLong() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], 0, 11);
+ }
+ }
+
+ @Test
+ public void testAvroFileInputStreamZeroLengthRead() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ assertEquals(0, is.read(new byte[10], 0, 0));
Review Comment:
prefer AssertJ for new code
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestTextCommand.java:
##########
@@ -47,13 +50,18 @@ public class TestTextCommand {
new File(TEST_ROOT_DIR, "weather.avro").toURI().getPath();
private static final String TEXT_FILENAME =
new File(TEST_ROOT_DIR, "testtextfile.txt").toURI().getPath();
+ private static final String SEQUENCE_FILENAME =
+ new File(TEST_ROOT_DIR, "NonWritableSequenceFile").toURI().getPath();
private static final String SEPARATOR = System.getProperty("line.separator");
+ @Rule
Review Comment:
if you make this a contract test, there's a rule in the superclass along
with some thread naming
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestTextCommand.java:
##########
@@ -67,39 +75,194 @@ public void testDisplayForAvroFiles() throws Exception {
assertEquals(expectedOutput, output);
}
+ @Test
+ public void testEmptyAvroFile() throws Exception {
+ String output = readUsingTextCommand(AVRO_FILENAME,
+ generateEmptyAvroBinaryData());
+ assertTrue(output.isEmpty());
+ }
+
+ @Test(expected = NullPointerException.class)
+ public void testAvroFileInputStreamNullBuffer() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(null, 0, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamNegativePosition() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], -1, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamTooLong() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], 0, 11);
+ }
+ }
+
+ @Test
+ public void testAvroFileInputStreamZeroLengthRead() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
Review Comment:
create a constant field for this as it is used everywhere
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Display.java:
##########
@@ -237,30 +237,69 @@ public TextRecordInputStream(FileStatus f) throws
IOException {
public int read() throws IOException {
int ret;
if (null == inbuf || -1 == (ret = inbuf.read())) {
- key = r.next(key);
- if (key == null) {
- return -1;
+ if (!readNextFromSequenceFile()) {
+ ret = -1;
} else {
- val = r.getCurrentValue(val);
+ ret = inbuf.read();
}
- byte[] tmp = key.toString().getBytes(StandardCharsets.UTF_8);
- outbuf.write(tmp, 0, tmp.length);
- outbuf.write('\t');
- tmp = val.toString().getBytes(StandardCharsets.UTF_8);
- outbuf.write(tmp, 0, tmp.length);
- outbuf.write('\n');
- inbuf.reset(outbuf.getData(), outbuf.getLength());
- outbuf.reset();
- ret = inbuf.read();
}
return ret;
}
+ @Override
+ public int read(byte[] dest, int destPos, int destLen) throws IOException {
+ if (dest == null) {
+ throw new NullPointerException();
Review Comment:
nit: add text to the message.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/shell/TestTextCommand.java:
##########
@@ -67,39 +75,194 @@ public void testDisplayForAvroFiles() throws Exception {
assertEquals(expectedOutput, output);
}
+ @Test
+ public void testEmptyAvroFile() throws Exception {
+ String output = readUsingTextCommand(AVRO_FILENAME,
+ generateEmptyAvroBinaryData());
+ assertTrue(output.isEmpty());
+ }
+
+ @Test(expected = NullPointerException.class)
+ public void testAvroFileInputStreamNullBuffer() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(null, 0, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamNegativePosition() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], -1, 10);
+ }
+ }
+
+ @Test(expected = IndexOutOfBoundsException.class)
+ public void testAvroFileInputStreamTooLong() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ is.read(new byte[10], 0, 11);
+ }
+ }
+
+ @Test
+ public void testAvroFileInputStreamZeroLengthRead() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ assertEquals(0, is.read(new byte[10], 0, 0));
+ }
+ }
+
+ @Test
+ public void testAvroFileInputStreamConsistentEOF() throws Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is = getInputStream(uri, conf)) {
+ inputStreamToString(is);
+ assertEquals(-1, is.read());
+ assertEquals(-1, is.read(new byte[10], 0, 10));
+ }
+ }
+
+ @Test
+ public void testAvroFileInputStreamSingleAndMultiByteReads() throws
Exception {
+ createFile(AVRO_FILENAME, generateWeatherAvroBinaryData());
+ URI uri = new URI(AVRO_FILENAME);
+ Configuration conf = new Configuration();
+ try (InputStream is1 = getInputStream(uri, conf);
+ InputStream is2 = getInputStream(uri, conf)) {
+ String multiByteReads = inputStreamToString(is1);
+ String singleByteReads = inputStreamSingleByteReadsToString(is2);
+ assertEquals(multiByteReads, singleByteReads);
+ }
+ }
+
/**
* Tests that a zero-length file is displayed correctly.
*/
- @Test (timeout = 30000)
- public void testEmptyTextFil() throws Exception {
+ @Test
+ public void testEmptyTextFile() throws Exception {
byte[] emptyContents = { };
String output = readUsingTextCommand(TEXT_FILENAME, emptyContents);
- assertTrue("".equals(output));
+ assertTrue(output.isEmpty());
Review Comment:
use assertJ
```
Assertions.assertThat(output)
.describeAs("output")
.isEmpty()
```
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/Display.java:
##########
@@ -286,31 +326,98 @@ public AvroFileInputStream(FileStatus status) throws
IOException {
writer = new GenericDatumWriter<Object>(schema);
output = new ByteArrayOutputStream();
encoder = EncoderFactory.get().jsonEncoder(schema, output);
+ finalSeparator =
System.getProperty("line.separator").getBytes(StandardCharsets.UTF_8);
}
/**
* Read a single byte from the stream.
*/
@Override
public int read() throws IOException {
+ if (buffer == null) {
+ return -1;
+ }
+
if (pos < buffer.length) {
return buffer[pos++];
}
+
if (!fileReader.hasNext()) {
+ // Unset buffer to signal EOF on future calls.
+ buffer = null;
return -1;
}
+
writer.write(fileReader.next(), encoder);
encoder.flush();
+
if (!fileReader.hasNext()) {
- // Write a new line after the last Avro record.
- output.write(System.getProperty("line.separator")
- .getBytes(StandardCharsets.UTF_8));
- output.flush();
+ if (buffer.length > 0) {
+ // Write a new line after the last Avro record.
+ output.write(finalSeparator);
+ output.flush();
+ }
+ }
+
+ swapBuffer();
+ return read();
+ }
+
+ @Override
+ public int read(byte[] dest, int destPos, int destLen) throws IOException {
+ if (dest == null) {
Review Comment:
bit of duplication -could be pulled out and used above too
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
