You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ZENOTME (via GitHub)" <gi...@apache.org> on 2024/05/21 08:20:20 UTC
[PR] feat: make file scan task serializable [iceberg-rust]
ZENOTME opened a new pull request, #377:
URL: https://github.com/apache/iceberg-rust/pull/377
There is a user case of file scan task for the compute engine:
1. compute the file scan task and shuffle them to the compute node
2. The compute node does the scan work in parallel
In this case, it required the compute engine could:
1. Access the `FileScanTask` directly
2. Serialize and Deserialize the `FileScanTask`
I draft this PR to try to make them accessible. Serialize and Deserialize `ManifestEntry` needs to take more work and I find that the reader only needs the file path in ManifestEntry. Seems the metadata in ManifestEntry is used in the planning phase to prune the file. After the plan is complete, lots of metadata is not needed. We can add the metadata we need for scanning in the future. So I make the FileScanTask to contain the data file path only. Please let me know if this assumption is wrong.
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#issuecomment-2123920187
This seems reasonable, but perhaps we might want to consider having this as a separate method to the existing `plan_files` though so that anyone who is using the existing stream of file plan tasks does not get broken by 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#issuecomment-2129114452
I think the idea looks good to me, but I'm not sure if we should modify `FileScanTask` now. Should we postpone this until we start to integrate with a true engine such as datafusion or ballista?
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#issuecomment-2132797415
I'm in favor of this change since this will keep the API much cleaner. The ManifestEntry will change over time, maybe good to keep Iceberg-specific things out of there. The pattern of the `FileScanTask` as an interface is proven by Iceberg both in Java as in Python.
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#issuecomment-2123974633
> This seems reasonable, but perhaps we might want to consider having this as a separate method to the existing `plan_files` though so that anyone who is using the existing stream of file plan tasks does not get broken by this.
I think we don't need a separate method for this.🤔 We just need to let `FileScanTask` be `Serialize, Deserialize` and then user can use the `plan_files()` to get the `FileScanTask` and transfer them to compute node. A simple case may like following to read all files at once. Also user can use the stream interface to have some optimization, e.g. read in stream way.
```
let plan_file_stream = scan.plan_files();
// read all file scan.
let file_scans = vec![];
#[for_await]
for file_scan in plan_file_stream {
file_scans.push(file_scan);
}
// send the file scan to the compute node. The compute node can
// read them all at once and use the Reader to read the data.
arrow_reader(stream::iter(file_scans.into_iter()))
```
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#issuecomment-2122049316
cc @liurenjie1024 @Fokko @Xuanwo @sdd @viirya
--
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@iceberg.apache.org
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
Re: [PR] feat: make file scan task serializable [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #377:
URL: https://github.com/apache/iceberg-rust/pull/377#discussion_r1611209532
##########
crates/iceberg/src/scan.rs:
##########
@@ -463,18 +464,19 @@ impl ManifestEvaluatorCache {
}
/// A task to scan part of file.
-#[derive(Debug)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct FileScanTask {
- data_manifest_entry: ManifestEntryRef,
+ data_file_path: String,
Review Comment:
This change makes a lot of sense to me. The statistics are used in the planning phase to filter out files where possible. The task gets handed over to the query engine where it will open up the actual file and there it can leverage the Parquet statistics to skip row groups and such.
The task should be extended with delete files (for example, based on the upper and lower bound we can efficiently remove unrelated positional deletes). Optional, but nice, a possibility of a residual predicate (for example, if you filter on `date(created_at) == '2024-03-01' and user_id = 123` then the first part of the predicate might be satisfied by the partitioning of the table, and we just need to filter on the `user_id`.
--
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@iceberg.apache.org
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