You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/28 03:10:54 UTC
[GitHub] [iceberg] wypoon opened a new pull request #2170: Make lookup of data file by file path robust.
wypoon opened a new pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170
This is a fix for [issue #2169](https://github.com/apache/iceberg/issues/2169).
In `BaseDataReader#getInputFile`, the `InputFile` is looked up in a map by the path string. The keys in the map are normalized paths. If the path string in the manifest is not normalized, the lookup will fail since the strings are not equal. The fix is to normalize the path string before looking it up in the map.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wypoon commented on a change in pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566487309
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
According to the javadoc for `InputFile#location`, this is "The fully-qualified location of the input file as a String." The only concern I have is if we ever call `FileIO#newInputFile` or `HadoopInputFile.fromLocation` with a location that is not fully-qualified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wypoon commented on a change in pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566495118
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,8 +71,7 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
Review comment:
Sure.
##########
File path: spark/src/main/java/org/apache/iceberg/spark/source/BaseDataReader.java
##########
@@ -125,7 +125,7 @@ public void close() throws IOException {
protected InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566493293
##########
File path: spark/src/main/java/org/apache/iceberg/spark/source/BaseDataReader.java
##########
@@ -125,7 +125,7 @@ public void close() throws IOException {
protected InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
Review comment:
I don't think this needs to change. Can you go back to the previous implementation?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566324656
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
Sounds like we will need to fix the interfaces that are modifying the file location and pass the original.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566260803
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
I don't think using the Hadoop API directly is a good way to solve the problem. It sounds like we need to fix the keys in the map to match the original location from the input split instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566326695
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
I think that `HadoopInputFile` just needs to be updated so that `fromLocation` preserves the original location rather than using `path.toString()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#issuecomment-769508048
Thanks, @wypoon! I'll merge this when tests are passing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566258898
##########
File path: spark/src/main/java/org/apache/iceberg/spark/source/BaseDataReader.java
##########
@@ -125,11 +126,13 @@ public void close() throws IOException {
protected InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
Review comment:
This isn't a Hadoop `Path`, it is a `CharSequence`. That's how we generally avoid issues like encoding.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wypoon commented on a change in pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566483173
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
@rdblue thank you for the suggestion; I have implemented it and updated the PR description accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] steveloughran commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
steveloughran commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566210134
##########
File path: spark/src/main/java/org/apache/iceberg/spark/source/BaseDataReader.java
##########
@@ -125,11 +126,13 @@ public void close() throws IOException {
protected InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
Review comment:
probably best to path().toURI() for best guarantee of handling of spaces
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#issuecomment-769957065
Merged. Thank you for fixing this, @wypoon! And thanks for the review, @steveloughran!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wypoon commented on a change in pull request #2170: Make lookup of data file by file path robust.
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566322395
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,12 +72,13 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
}
InputFile getInputFile(String location) {
- return inputFiles.get(location);
+ // normalize the path before looking it up in the map
+ Path path = new Path(location);
Review comment:
For @steveloughran's information, the reason the keys in the `inputFiles` map became normalized paths is that the files for the scan tasks go through encryption and decryption:
```
Stream<EncryptedInputFile> encrypted = keyMetadata.entrySet().stream()
.map(entry -> EncryptedFiles.encryptedInput(io.newInputFile(entry.getKey()), entry.getValue()));
// decrypt with the batch call to avoid multiple RPCs to a key server, if possible
Iterable<InputFile> decryptedFiles = encryptionManager.decrypt(encrypted::iterator);
```
and the keys are the location of the decrypted files. The call to `io.newInputFile` either passes through `HadoopFileIO` or `S3FileIO` (currently the two implementations of `FileIO`), and the `newInputFile` process normalized the path. The normalization through `HadoopFileIO` is exactly what I'm replicating here. I think it will work for an S3 path too.
@rdblue, from the decrypted files, I do not see a natural way to recover the original path string written in the manifest. Instead, can we add a method to the `FileIO` interface to return the normalized path for a path String, and then `HadoopFileIO` and `S3FileIO` will have to implement it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2170: Keep original location in HadoopInputFile if given
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2170:
URL: https://github.com/apache/iceberg/pull/2170#discussion_r566493109
##########
File path: flink/src/main/java/org/apache/iceberg/flink/source/DataIterator.java
##########
@@ -71,8 +71,7 @@
InputFile getInputFile(FileScanTask task) {
Preconditions.checkArgument(!task.isDataTask(), "Invalid task type");
-
- return inputFiles.get(task.file().path().toString());
+ return getInputFile(task.file().path().toString());
Review comment:
Looks like this doesn't need to change. Can you revert this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org