Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

minor: fix typos in comments / structure names #13879

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

zhuliquan
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

fix some typo error in doc

What changes are included in this PR?

fix some typo error in doc

Are these changes tested?

Are there any user-facing changes?

no

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait common Related to common crate proto Related to proto crate functions labels Dec 22, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contrbibution @zhuliquan - this looks like a great improvement ❤️ and will make the code easier to read and understand,.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically thiserror refers to https://crates.io/crates/thiserror

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I got it, I will recover it

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually meant to be something different:

Suggested change
// during query, this example loaded the Page Index pre-emptily by setting
// during query, this example loaded the Page Index preemptively by setting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I got it, I will recover it

@@ -115,7 +115,7 @@ pub trait Accumulator: Send + Sync + Debug {
/// │ │
/// │ │
/// ┌─────────────────────────┐ ┌─────────────────────────┐
/// │ GroubyBy │ │ GroubyBy
/// │ GroupBy │ │ GroupBy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

@@ -36,7 +36,7 @@ use datafusion_expr::{
TableScan, Window,
};

use crate::optimize_projections::required_indices::RequiredIndicies;
use crate::optimize_projections::required_indices::RequiredIndices;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above -- this is an internal struct so renaming it is not an API change

@alamb alamb changed the title minor: fix typo error in datafusion minor: fixs typos in comments / structure names Dec 22, 2024
@alamb alamb added the documentation Improvements or additions to documentation label Dec 22, 2024
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Dec 22, 2024
@alamb alamb changed the title minor: fixs typos in comments / structure names minor: fix typos in comments / structure names Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate functions logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions proto Related to proto crate sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants