wiedld commented on code in PR #7379: URL: https://github.com/apache/arrow-datafusion/pull/7379#discussion_r1330504324
########## datafusion/physical-plan/src/sorts/batch_cursor.rs: ########## @@ -0,0 +1,89 @@ +// 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. + +use datafusion_common::Result; + +use super::cursor::Cursor; + +pub type BatchId = u64; + +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] +pub struct BatchOffset(pub usize); + +pub type SlicedBatchCursorIdentifier = (BatchId, BatchOffset); + +/// The [`BatchCursor`] represents a complete, or partial, [`Cursor`] for a given record batch ([`BatchId`]). +/// +/// A record batch (represented by its [`Cursor`]) can be sliced due to the following reason: +/// 1. a merge node takes in 10 streams +/// 2 at any given time, this means up to 10 cursors (record batches) are being merged (e.g. in the loser tree) +/// 3. merge nodes will yield once it hits a size limit +/// 4. at the moment of yielding, there may be some cursors which are partially yielded +/// +/// Unique representation of sliced cursor is denoted by the [`SlicedBatchCursorIdentifier`]. +#[derive(Debug)] +pub struct BatchCursor<C: Cursor> { Review Comment: (moved comment, after main merge conflict-resolution): This is used in the CursorStream (not the BatchCursorStream which included the actual record batches). Therefore, I think this could have a better name. It wraps the cursor, and maps it to the original (tracked) batch -- as well as tracking the sliced offset. Naming ideas? ########## datafusion/core/src/physical_plan/sorts/batch_cursor.rs: ########## @@ -0,0 +1,89 @@ +// 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. + +use datafusion_common::Result; + +use super::cursor::Cursor; + +pub type BatchId = u64; + +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] +pub struct BatchOffset(pub usize); + +pub type SlicedBatchCursorIdentifier = (BatchId, BatchOffset); + +/// The [`BatchCursor`] represents a complete, or partial, [`Cursor`] for a given record batch ([`BatchId`]). +/// +/// A record batch (represented by its [`Cursor`]) can be sliced due to the following reason: +/// 1. a merge node takes in 10 streams +/// 2 at any given time, this means up to 10 cursors (record batches) are being merged (e.g. in the loser tree) +/// 3. merge nodes will yield once it hits a size limit +/// 4. at the moment of yielding, there may be some cursors which are partially yielded +/// +/// Unique representation of sliced cursor is denoted by the [`SlicedBatchCursorIdentifier`]. +#[derive(Debug)] +pub struct BatchCursor<C: Cursor> { Review Comment: This is used in the `CursorStream` (not the `BatchCursorStream` which included the actual record batches). Therefore, I think this could have a better name. It wraps the cursor, and maps it to the original (tracked) batch -- as well as tracking the sliced offset. Naming ideas? -- 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]
