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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
