-
Notifications
You must be signed in to change notification settings - Fork 3.4k
core: Adding read vector to range readable interface and adding mappe… #13997
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.iceberg.io; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
| import java.util.concurrent.CompletableFuture; | ||
|
|
||
| public class FileRange { | ||
| private final CompletableFuture<ByteBuffer> byteBuffer; | ||
| private final long offset; | ||
| private final int length; | ||
|
|
||
| public FileRange(CompletableFuture<ByteBuffer> byteBuffer, long offset, int length) { | ||
| this.byteBuffer = byteBuffer; | ||
| this.offset = offset; | ||
| this.length = length; | ||
| } | ||
|
|
||
| public CompletableFuture<ByteBuffer> byteBuffer() { | ||
| return byteBuffer; | ||
| } | ||
|
|
||
| public long offset() { | ||
| return offset; | ||
| } | ||
|
|
||
| public int length() { | ||
| return length; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.iceberg.util; | ||
|
|
||
| import java.io.EOFException; | ||
| import java.util.Comparator; | ||
| import java.util.List; | ||
| import org.apache.iceberg.io.FileRange; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Preconditions; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Lists; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Utils class for vectoredReads, to help with things like range validation. Most of the code in | ||
| * this class is written by @mukundthakur, and taken from | ||
| * /hadoop-common/src/main/java/org/apache/hadoop/fs/VectoredReadUtils.java (thank you!). | ||
| */ | ||
| public final class VectoredReadUtils { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel like we need this class. There are three things this does, but it should probalby be just one. The I'd suggest moving |
||
| private VectoredReadUtils() {} | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(VectoredReadUtils.class); | ||
|
|
||
| /** | ||
| * Validate a list of ranges (including overlapping checks) and return the sorted list. | ||
| * | ||
| * <p>Two ranges overlap when the start offset of second is less than the end offset of first. End | ||
| * offset is calculated as start offset + length. | ||
| * | ||
| * @param input input list | ||
| * @return a new sorted list. | ||
| * @throws IllegalArgumentException if there are overlapping ranges or a range element is invalid | ||
| * (other than with negative offset) | ||
| * @throws EOFException if the last range extends beyond the end of the file supplied or a range | ||
| * offset is negative | ||
| */ | ||
| public static List<FileRange> validateAndSortRanges(final List<FileRange> input) | ||
| throws EOFException { | ||
|
|
||
| Preconditions.checkNotNull(input, "Null input list"); | ||
|
|
||
| if (input.isEmpty()) { | ||
| // this may seem a pathological case, but it was valid | ||
| // before and somehow Spark can call it through parquet. | ||
| LOG.debug("Empty input list"); | ||
| return input; | ||
| } | ||
|
|
||
| final List<FileRange> sortedRanges; | ||
|
|
||
| if (input.size() == 1) { | ||
| validateRangeRequest(input.get(0)); | ||
| sortedRanges = input; | ||
| } else { | ||
| sortedRanges = sortRangeList(input); | ||
| FileRange prev = null; | ||
| for (final FileRange current : sortedRanges) { | ||
| validateRangeRequest(current); | ||
| if (prev != null) { | ||
| Preconditions.checkArgument( | ||
| current.offset() >= prev.offset() + prev.length(), | ||
| "Overlapping ranges %s and %s", | ||
| prev, | ||
| current); | ||
| } | ||
| prev = current; | ||
| } | ||
| } | ||
|
|
||
| return sortedRanges; | ||
| } | ||
|
|
||
| /** | ||
| * Validate a single range. | ||
| * | ||
| * @param range range to validate. | ||
| * @return the range. | ||
| * @throws IllegalArgumentException the range length is negative or other invalid condition is met | ||
| * other than the those which raise EOFException or NullPointerException. | ||
| * @throws EOFException the range offset is negative | ||
| * @throws NullPointerException if the range is null. | ||
| */ | ||
| public static FileRange validateRangeRequest(FileRange range) throws EOFException { | ||
| Preconditions.checkNotNull(range, "range is null"); | ||
|
|
||
| Preconditions.checkArgument(range.length() >= 0, "length is negative in %s", range); | ||
| if (range.offset() < 0) { | ||
| throw new EOFException("position is negative in range " + range); | ||
| } | ||
| return range; | ||
| } | ||
|
|
||
| /** | ||
| * Sort the input ranges by offset; no validation is done. | ||
| * | ||
| * @param input input ranges. | ||
| * @return a new list of the ranges, sorted by offset. | ||
| */ | ||
| public static List<FileRange> sortRangeList(List<FileRange> input) { | ||
| final List<FileRange> l = Lists.newArrayList(input); | ||
| l.sort(Comparator.comparingLong(FileRange::offset)); | ||
| return l; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1340,6 +1340,7 @@ public <D> CloseableIterable<D> build() { | |
| optionsBuilder.withDecryption(fileDecryptionProperties); | ||
| } | ||
|
|
||
| optionsBuilder.withUseHadoopVectoredIo(true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There were some efforts to allow Iceberg working without Hadoop on the classpath. Could you please help me understand the consequences of always using Thanks,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For part 1 about the effort to reduce the dependencies on Hadoop I don't think that was ever completed I do see a TODO comment about wanting to do it. I am probably making the effort more complicated as I am adding 2 new imports from Hadoop but I don't think that is a big risk. for 2) withUseHadoopVectoredIo is used in the file reader in conjunction with readVectoredAvailable() so moving to always using readVector doesn't change anything unless the stream also supports readVectored.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the naming of this option is a little misleading. The |
||
| ParquetReadOptions options = optionsBuilder.build(); | ||
|
|
||
| NameMapping mapping; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,11 @@ | |
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.nio.ByteBuffer; | ||
| import java.util.List; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.function.IntFunction; | ||
| import java.util.stream.Collectors; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FSDataInputStream; | ||
| import org.apache.hadoop.fs.FSDataOutputStream; | ||
|
|
@@ -29,11 +34,15 @@ | |
| import org.apache.iceberg.hadoop.HadoopOutputFile; | ||
| import org.apache.iceberg.io.DelegatingInputStream; | ||
| import org.apache.iceberg.io.DelegatingOutputStream; | ||
| import org.apache.iceberg.io.FileRange; | ||
| import org.apache.iceberg.io.RangeReadable; | ||
| import org.apache.parquet.bytes.ByteBufferAllocator; | ||
| import org.apache.parquet.hadoop.util.HadoopStreams; | ||
| import org.apache.parquet.io.DelegatingPositionOutputStream; | ||
| import org.apache.parquet.io.DelegatingSeekableInputStream; | ||
| import org.apache.parquet.io.InputFile; | ||
| import org.apache.parquet.io.OutputFile; | ||
| import org.apache.parquet.io.ParquetFileRange; | ||
| import org.apache.parquet.io.PositionOutputStream; | ||
| import org.apache.parquet.io.SeekableInputStream; | ||
|
|
||
|
|
@@ -91,6 +100,9 @@ static SeekableInputStream stream(org.apache.iceberg.io.SeekableInputStream stre | |
| return HadoopStreams.wrap((FSDataInputStream) wrapped); | ||
| } | ||
| } | ||
| if (stream instanceof RangeReadable) { | ||
|
stubz151 marked this conversation as resolved.
|
||
| return new ParquetRangeReadableInputStreamAdapter(stream); | ||
| } | ||
|
stubz151 marked this conversation as resolved.
|
||
| return new ParquetInputStreamAdapter(stream); | ||
| } | ||
|
|
||
|
|
@@ -123,6 +135,64 @@ public void seek(long newPos) throws IOException { | |
| } | ||
| } | ||
|
|
||
| private static class ParquetRangeReadableInputStreamAdapter< | ||
| T extends org.apache.iceberg.io.SeekableInputStream & RangeReadable> | ||
| extends DelegatingSeekableInputStream implements RangeReadable { | ||
| private final T delegate; | ||
|
|
||
| private ParquetRangeReadableInputStreamAdapter(T delegate) { | ||
| super(delegate); | ||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| @Override | ||
| public long getPos() throws IOException { | ||
| return delegate.getPos(); | ||
| } | ||
|
|
||
| @Override | ||
| public void seek(long newPos) throws IOException { | ||
| delegate.seek(newPos); | ||
| } | ||
|
|
||
| @Override | ||
| public void readFully(long position, byte[] buffer, int offset, int length) throws IOException { | ||
| delegate.readFully(position, buffer, offset, length); | ||
| } | ||
|
|
||
| @Override | ||
| public int readTail(byte[] buffer, int offset, int length) throws IOException { | ||
| return delegate.readTail(buffer, offset, length); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean readVectoredAvailable(ByteBufferAllocator allocate) { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public void readVectored(List<ParquetFileRange> ranges, ByteBufferAllocator allocate) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some tests at the ParquetIO level to validate this? I know we're adding some in S3FileIO, but it would be good to have this interface tested (even if there's a mock implementation)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added in testRangeReadableAdapterReadVectored which does something similar to the tests in S3FileIO, but focused a bit more on checking that the buffers/ranges are being used correctly, I skipped the other operations but can add them in if we want. Let me know |
||
| throws IOException { | ||
| IntFunction<ByteBuffer> delegateAllocate = (allocate::allocate); | ||
| List<FileRange> delegateRange = convertRanges(ranges); | ||
|
danielcweeks marked this conversation as resolved.
|
||
| delegate.readVectored(delegateRange, delegateAllocate); | ||
| } | ||
|
|
||
| private static List<FileRange> convertRanges(List<ParquetFileRange> ranges) { | ||
| return ranges.stream() | ||
| .map( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this just maps between the internal parquet hadoop range and the new iceberg one. |
||
| parquetFileRange -> { | ||
| CompletableFuture<ByteBuffer> result = new CompletableFuture<>(); | ||
| parquetFileRange.setDataReadFuture(result); | ||
|
stubz151 marked this conversation as resolved.
Outdated
|
||
| return new FileRange( | ||
| parquetFileRange.getDataReadFuture(), | ||
| parquetFileRange.getOffset(), | ||
| parquetFileRange.getLength()); | ||
| }) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| } | ||
|
|
||
| private static class ParquetOutputStreamAdapter extends DelegatingPositionOutputStream { | ||
| private final org.apache.iceberg.io.PositionOutputStream delegate; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the parquet implementation, I don't think you can pass the byteBuffer future in like this. I believe this is intended to be set by the implementation so that it can be returned to the invoker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We not passing the bytebuffer in here right, we passing a future that completes with a byte buffer, we need a way to map the futures in Iceberg to the future's we are setting in Parquet,
So when we call
parquetFileRange.setDataReadFuture(future);we need to have a way of tracking that future in Iceberg and that's what this gives us.