alamb commented on code in PR #13879: URL: https://github.com/apache/datafusion/pull/13879#discussion_r1894927649
########## datafusion/expr-common/src/accumulator.rs: ########## @@ -115,7 +115,7 @@ pub trait Accumulator: Send + Sync + Debug { /// │ │ /// │ │ /// ┌─────────────────────────┐ ┌─────────────────────────┐ - /// │ GroubyBy │ │ GroubyBy │ + /// │ GroupBy │ │ GroupBy │ Review Comment: 🤦 ########## datafusion/optimizer/src/optimize_projections/mod.rs: ########## @@ -36,7 +36,7 @@ use datafusion_expr::{ TableScan, Window, }; -use crate::optimize_projections::required_indices::RequiredIndicies; +use crate::optimize_projections::required_indices::RequiredIndices; Review Comment: Since this is an internal struct (not exposed publically) I think this is not an API change: https://docs.rs/datafusion/latest/datafusion/index.html?search=RequiredIndicies ########## datafusion-examples/examples/advanced_parquet_index.rs: ########## @@ -211,7 +211,7 @@ async fn main() -> Result<()> { // // Note: in order to prune pages, the Page Index must be loaded and the // ParquetExec will load it on demand if not present. To avoid a second IO - // during query, this example loaded the Page Index pre-emptively by setting + // during query, this example loaded the Page Index pre-emptily by setting Review Comment: I think this is actually meant to be something different: ```suggestion // during query, this example loaded the Page Index preemptively by setting ``` ########## datafusion/optimizer/src/optimize_projections/required_indices.rs: ########## @@ -35,15 +35,15 @@ use datafusion_expr::{Expr, LogicalPlan}; /// indices were added `[3, 2, 4, 3, 6, 1]`, the instance would be represented /// by `[1, 2, 3, 4, 6]`. #[derive(Debug, Clone, Default)] -pub(super) struct RequiredIndicies { +pub(super) struct RequiredIndices { Review Comment: See above -- this is an internal struct so renaming it is not an API change ########## datafusion-cli/src/main.rs: ########## @@ -209,7 +209,7 @@ async fn main_inner() -> Result<()> { if !rc.is_empty() { exec::exec_from_files(&ctx, rc, &print_options).await?; } - // TODO maybe we can have thiserror for cli but for now let's keep it simple + // TODO maybe we can have this error for cli but for now let's keep it simple Review Comment: I think technically thiserror refers to https://crates.io/crates/thiserror -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org