alamb commented on code in PR #9662: URL: https://github.com/apache/arrow-datafusion/pull/9662#discussion_r1530851819
########## datafusion/physical-plan/src/aggregates/group_values/fully_ordered.rs: ########## @@ -0,0 +1,177 @@ +// 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 crate::aggregates::group_values::GroupValues; +use crate::common::transpose; +use arrow::compute::{cast, SortColumn}; +use arrow::record_batch::RecordBatch; +use arrow_array::{Array, ArrayRef}; +use arrow_schema::{DataType, SchemaRef}; +use datafusion_common::utils::{evaluate_partition_ranges, get_row_at_idx}; +use datafusion_common::{DataFusionError, Result, ScalarValue}; +use datafusion_expr::EmitTo; +use datafusion_physical_expr::LexOrdering; + +/// A [`GroupValues`] making use of order Review Comment: Can we please expand this description (perhaps using some of the content in the description of this PR Notable points include that the input to the group values are sure to be totally ordered ########## datafusion/physical-plan/src/aggregates/group_values/fully_ordered.rs: ########## @@ -0,0 +1,177 @@ +// 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 crate::aggregates::group_values::GroupValues; +use crate::common::transpose; +use arrow::compute::{cast, SortColumn}; +use arrow::record_batch::RecordBatch; +use arrow_array::{Array, ArrayRef}; +use arrow_schema::{DataType, SchemaRef}; +use datafusion_common::utils::{evaluate_partition_ranges, get_row_at_idx}; +use datafusion_common::{DataFusionError, Result, ScalarValue}; +use datafusion_expr::EmitTo; +use datafusion_physical_expr::LexOrdering; + +/// A [`GroupValues`] making use of order +pub struct GroupValuesFullyOrdered { + /// The output schema + schema: SchemaRef, + + /// The actual group by values, stored as `Vec<ScalarValue>` for each distinct group. Review Comment: The idea that the overhead of using `Vec<ScalarValue>` to store the row is better than having to convert all the rows into `RowFormat` I think makes sense for low cardinality (when there are relatively few groups) but for high cardinality (when there are many distinct groups) this may not hold I ########## datafusion/physical-plan/src/aggregates/group_values/fully_ordered.rs: ########## @@ -0,0 +1,177 @@ +// 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 crate::aggregates::group_values::GroupValues; +use crate::common::transpose; +use arrow::compute::{cast, SortColumn}; +use arrow::record_batch::RecordBatch; +use arrow_array::{Array, ArrayRef}; +use arrow_schema::{DataType, SchemaRef}; +use datafusion_common::utils::{evaluate_partition_ranges, get_row_at_idx}; +use datafusion_common::{DataFusionError, Result, ScalarValue}; +use datafusion_expr::EmitTo; +use datafusion_physical_expr::LexOrdering; + +/// A [`GroupValues`] making use of order +pub struct GroupValuesFullyOrdered { + /// The output schema + schema: SchemaRef, + + /// The actual group by values, stored as `Vec<ScalarValue>` for each distinct group. Review Comment: Another possibility might be to use something like the `RecordBatchStore` from the TopK implementation: https://github.com/apache/arrow-datafusion/blob/09747596fd75bfce8903e86472cccb8acc524453/datafusion/physical-plan/src/topk/mod.rs#L537-L551 And totally avoid copying rows at all 🤔 -- 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]
