Use memory-backed streams in tests #4534

findepi · 2022-04-11T10:05:38Z

Sometimes an actual file is needed, but sometimes it's not. In the case
where it isn't, we can use an in-memory InputFile, OutputFile
implementation.

Extracted from #4537

rdblue · 2022-04-12T02:45:41Z

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

+import java.util.UUID;
+
+import static java.util.Objects.requireNonNull;
+import static org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkState;


We typically don't use static method imports. I think they are mostly disallowed by the Iceberg style enforcement.

I noticed the ~~Test*~~ test classes are exempt from the checks. I will fix this one nonetheless.

findepi · 2022-04-12T07:26:43Z

AC

kbendick

Thanks @findepi! I know this is something that has been considered before, so it's good to see it being added.

Hopefully this will make tests that much faster. Left some style notes and a few comments.

kbendick · 2022-04-14T16:19:24Z

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

+    this.location = Objects.requireNonNull(location, "location is null");
+    this.contents = Objects.requireNonNull(contents, "contents is null").clone();


Usually we use Preconditions.checkNotNull, but since this code is in test the style rules are somewhat more lax.

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

kbendick · 2022-04-14T16:26:42Z

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

+    public boolean markSupported() {
+      throw new UnsupportedOperationException();
+    }


Nit: Is there a benefit to throwing UnsupportedOperationException or should this return false?

core/src/test/java/org/apache/iceberg/io/InMemoryOutputFile.java

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

kbendick · 2022-04-14T17:04:09Z

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

+    public void seek(long newPos) throws IOException {
+      delegate.reset();
+      Preconditions.checkState(delegate.skip(newPos) == newPos,
+          "Invalid position %s within stream of length %s", newPos, length);
+    }


Nit: We should consider adding an explicit check that the new position is non-negative.

The checkState call seems like it will cover that as skip shouldn't return a negative number, but it might be better for tests to add an explicit check. WDYT?

I think that it's just tests, so exception message doesn't matter that much, and adding another check doesn't make the existing check unnecessary. i would leave this as is, if you don't mind

kbendick · 2022-04-14T17:08:54Z

core/src/test/java/org/apache/iceberg/io/InMemoryOutputFile.java

+
+  private final String location;
+
+  private boolean exists;


Do we explicitly set exists to false anywhere or are we relying on the default value being false?

I'd prefer we explicitly set the value false somewhere for readability purposes.

Added an explicit initializer.

Note that in Trino we have a hard rule that a field must not have an explicit initializer that is same as the default value for given type, so i am afraid I will need to be reminded about this from time to time. Please bear with me

kbendick · 2022-04-14T17:13:01Z

core/src/test/java/org/apache/iceberg/io/InMemoryOutputFile.java

+  @Override
+  public PositionOutputStream create() {
+    if (exists) {
+      throw new RuntimeException("Already exists");


Can we throw AlreadyExistsException to match the current LocalOutputFile#create() method?

iceberg/api/src/main/java/org/apache/iceberg/Files.java

Lines 57 to 59 in e45c19e

if (file.exists()) {

throw new AlreadyExistsException("File already exists: %s", file);

}

Thanks, indeed this is what interface requires me to do

core/src/test/java/org/apache/iceberg/io/InMemoryOutputFile.java

RussellSpitzer · 2022-04-14T17:32:10Z

core/src/test/java/org/apache/iceberg/TestManifestWriterVersions.java


  private InputFile writeManifestList(ManifestFile manifest, int formatVersion) throws IOException {
-    OutputFile manifestList = Files.localOutput(temp.newFile());
+    InMemoryOutputFile manifestList = new InMemoryOutputFile();


Could just still be OutputFile right?

~~reverted~~

yes, fixed (not reverted)

RussellSpitzer · 2022-04-14T17:34:28Z

core/src/test/java/org/apache/iceberg/avro/TestGenericAvro.java

-    Assert.assertTrue("Delete should succeed", testFile.delete());
-
-    try (FileAppender<Record> writer = Avro.write(Files.localOutput(testFile))
+    InMemoryOutputFile outputFile = new InMemoryOutputFile();


Could just be output File

RussellSpitzer

This makes sense to me, although i was wondering when I saw the title whether you were going to implement a full FileIO since we could be using that now. I think this is fine for these tests though

kbendick

Outside of the out of date comment related to mark and the question around throwing for markSupported, this looks good for the current testing purposes.

My other comments are more informational or for clarification.

findepi · 2022-04-15T12:25:11Z

This makes sense to me, although i was wondering when I saw the title whether you were going to implement a full FileIO since we could be using that now. I think this is fine for these tests though

Not until i need that :)

findepi · 2022-04-15T12:49:55Z

AC

kbendick · 2022-04-15T17:16:03Z

core/src/test/java/org/apache/iceberg/io/InMemoryInputFile.java

+  public InMemoryInputFile(String location, byte[] contents) {
+    Preconditions.checkNotNull(location, "location is null");
+    Preconditions.checkNotNull(contents, "contents is null");
+    this.location = location;
+    this.contents = contents.clone();
+  }


As a side note: Typically we use ByteBuffers instead of raw byte[], so that we can get views over slices etc for free and help avoid additional allocations in many situations.

ByteBuffers also have the advantage of presenting views over type-specific byte arrays (for char, int, float, etc). And these views are indexed in terms of the type-specific size of the values, which is easier to reason about and also have bulk get and set methods.

This is test code, but if we were to ever move this to be code used in the project, we'd almost certainly want to use ByteBuffer instead of the raw byte[] throughout where possible.

For internal representation byte[] is preferred, since the i can ByteArray{Output,Input}Stream}.
For constructor -- whatever it's easier to provide by the caller. I think we eventually will accept both.

Yeah I'm good with that. If this were ported outside of test, I just wanted to make sure you were aware that request might come up (though it might not depending on the situation) 🙂

Sometimes an actual file is needed, but sometimes it's not. In the case where it isn't, we can use an in-memory `InputFile`, `OutputFile` implementation.

findepi · 2022-04-19T10:21:39Z

AC

All comments applied or answered to.
Do you want me to mark them as resolved?

@RussellSpitzer @kbendick do you mind to take another look and let me know what else I could change here?

findepi · 2022-04-20T09:21:24Z

@RussellSpitzer @rdblue can you please give me more advice here?

kbendick

Reconfirming my +1. This looks good to me. Thanks @findepi!

RussellSpitzer · 2022-04-21T19:52:09Z

Thanks @findepi for the contribution and @kbendick and @rdblue for review

findepi · 2022-04-22T07:33:24Z

thank you for the merge!

github-actions bot added the core label Apr 11, 2022

findepi requested review from RussellSpitzer, danielcweeks, openinx and rdblue April 11, 2022 17:53

rdblue reviewed Apr 12, 2022

View reviewed changes

findepi force-pushed the findepi/use-memory-backed-streams-in-tests-3b5a04 branch from 45c90c4 to 201eb2a Compare April 12, 2022 07:26

findepi requested a review from rdblue April 12, 2022 07:26

findepi force-pushed the findepi/use-memory-backed-streams-in-tests-3b5a04 branch from 201eb2a to d9b424f Compare April 12, 2022 07:41

kbendick reviewed Apr 14, 2022

View reviewed changes

RussellSpitzer reviewed Apr 14, 2022

View reviewed changes

RussellSpitzer approved these changes Apr 14, 2022

View reviewed changes

kbendick approved these changes Apr 14, 2022

View reviewed changes

findepi force-pushed the findepi/use-memory-backed-streams-in-tests-3b5a04 branch from d9b424f to 9e7a4f5 Compare April 15, 2022 12:25

kbendick reviewed Apr 15, 2022

View reviewed changes

Use memory-backed streams in tests

370672a

Sometimes an actual file is needed, but sometimes it's not. In the case where it isn't, we can use an in-memory `InputFile`, `OutputFile` implementation.

findepi force-pushed the findepi/use-memory-backed-streams-in-tests-3b5a04 branch from 9e7a4f5 to 370672a Compare April 19, 2022 10:20

findepi requested review from RussellSpitzer and kbendick April 19, 2022 10:21

findepi mentioned this pull request Apr 20, 2022

Add reader and writer for Puffin, indexes and stats file format #4537

Merged

RussellSpitzer approved these changes Apr 20, 2022

View reviewed changes

kbendick approved these changes Apr 21, 2022

View reviewed changes

RussellSpitzer merged commit ed54952 into apache:master Apr 21, 2022

findepi deleted the findepi/use-memory-backed-streams-in-tests-3b5a04 branch April 22, 2022 07:33

rdblue mentioned this pull request Apr 24, 2022

Core: Add ByteBufferInputStream and implementations #4623

Merged

		this.location = Objects.requireNonNull(location, "location is null");
		this.contents = Objects.requireNonNull(contents, "contents is null").clone();

	if (file.exists()) {
	throw new AlreadyExistsException("File already exists: %s", file);
	}

Use memory-backed streams in tests #4534

Use memory-backed streams in tests #4534

Uh oh!

Conversation

findepi commented Apr 11, 2022 • 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!

findepi Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Apr 12, 2022

Uh oh!

kbendick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

kbendick left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Apr 15, 2022

Uh oh!

findepi commented Apr 15, 2022

Uh oh!

kbendick Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

findepi commented Apr 19, 2022

Uh oh!

findepi commented Apr 20, 2022

Uh oh!

kbendick left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Apr 21, 2022

Uh oh!

findepi commented Apr 22, 2022

Uh oh!

Reviewers

findepi commented Apr 11, 2022 •

edited

Loading

findepi Apr 12, 2022 •

edited

Loading

findepi Apr 15, 2022 •

edited

Loading

kbendick Apr 15, 2022 •

edited

Loading