chore: fix typos #17135

waynexia · 2025-08-12T02:42:36Z

Signed-off-by: Ruihang Xia [email protected]## Which issue does this PR close?

Inspired by speedup date_trunc (~7x faster) in some cases #16859 (comment)

Rationale for this change

From #16859 (comment), then I think why not add an automatic checker

What changes are included in this PR?

A typo checker, and fix all existing typos (or allowlist them). It runs like https://github.com/waynexia/arrow-datafusion/actions/runs/16897471061/job/47869907355

Are these changes tested?

no

Are there any user-facing changes?

API change:

SplitMetrics::batches_splitted field is renamed to SplitMetrics::batches_split (grammar typo #17135 (comment))

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2025-08-12T02:43:37Z

typos.toml

+alph = "alph"
+wih = "wih"
+Ded = "Ded"
+Serializeable = "Serializeable"


This is a public interface so I added it to allowlist

Does the checker have a way to exclude given work at given use place? or only global excludes are available?

We can specify the scope https://github.com/crate-ci/typos#false-positives. This might be useful to those from test text.

waynexia · 2025-08-12T02:44:17Z

typos.toml

+Pn = "Pn"
+fo = "fo"
+flate = "flate"
+nd = "nd"
+Nd = "Nd"
+YOUY = "YOUY"
+typ = "typ"
+ba = "ba"
+lits = "lits"
+ECT = "ECT"
+Ue = "Ue"
+Iy = "Iy"
+hte = "hte"
+numer = "numer"
+abd = "abd"
+aroun = "aroun"
+carefull = "carefull"
+abov = "abov"
+Ois = "Ois"
+alo = "alo"
+precentage = "precentage"
+datas = "datas"
+hom = "hom"
+alph = "alph"
+wih = "wih"
+Ded = "Ded"


Most of them are from tests or benchmarks

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2025-08-12T03:26:35Z

.github/workflows/rust.yml

+    name: Spell Check with Typos
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683


pin to a fixed revision to prevent it from breaking unexpectedly because of a dictionary update. And use an unversioned new commit.

if "unversioned new commit." is something not released yet, then let's please use a tagged version

See also #17046 (comment)
I believe there is no good reason to pin precise commits for GitHub's own internal actions such as actions/checkout.

zhuqi-lucas

LGTM , thank you @waynexia , very good improvement to me!

findepi

Love it! ❤️

This PR is quite a lot of changes.
Could you please send typo fixes in PR separate from spellchecker workflow?
Typo fixes require low scrutiny (mostly checks for API components), which is different from GH actions.

findepi · 2025-08-12T09:37:12Z

.github/workflows/rust.yml

+    name: Spell Check with Typos
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683


if "unversioned new commit." is something not released yet, then let's please use a tagged version

See also #17046 (comment)
I believe there is no good reason to pin precise commits for GitHub's own internal actions such as actions/checkout.

.github/workflows/rust.yml

findepi · 2025-08-12T09:39:23Z

.github/workflows/rust.yml

+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+        with:
+          persist-credentials: false
+      - uses: crate-ci/typos@master


Is this a 3rd party action maintained at https://github.com/crate-ci/typos?
Is it already ASF approved?
If yes, this must pin to a particular commit hash.

find it https://github.com/apache/infrastructure-actions/blob/4b7c80ef656dd3a147546ac60c4e10c0285f0927/approved_patterns.yml#L82. I'll use a version (or hash) in the follow up PR

Cool file! Is there ASF-maintained workflow or action to validate our workflows using the approved_patterns file?

Or maybe it's already the case (as suggested by https://github.com/apache/infrastructure-actions/tree/4b7c80ef656dd3a147546ac60c4e10c0285f0927?tab=readme-ov-file#management-of-organization-wide-github-actions-allow-list)?

I'm not very sure about this part... actually I doubt how this is enforced 🙈

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2025-08-12T22:15:14Z

Thank you!

I removed the job from this PR. I'll check if it's permitted while waiting for this PR to merge, and file another one dedicated for the checker

findepi

LGTM % few places.

findepi · 2025-08-13T09:20:45Z

datafusion/core/tests/fuzz_cases/aggregate_fuzz.rs

@@ -403,7 +403,7 @@ async fn run_aggregate_test(input1: Vec<RecordBatch>, group_by_columns: Vec<&str
             Left Plan:\n{}\n\
             Right Plan:\n{}\n\
             schema:\n{schema}\n\
-             Left Ouptut:\n{}\n\


findepi · 2025-08-13T09:22:37Z

datafusion/physical-expr/src/async_scalar_function.rs

@@ -192,13 +192,14 @@ impl AsyncFuncExpr {
            );
        }

-        let datas = ColumnarValue::values_to_arrays(&result_batches)?
+        let data_vec = ColumnarValue::values_to_arrays(&result_batches)?


there is no such word datas, but it's likely as meaningful as data_vec
it could be allowed (no need to change anything)

Yes, I then found there are many datas so I added it to the allowlist... will revert this as well

findepi · 2025-08-13T09:27:21Z

datafusion/physical-plan/src/metrics/baseline.rs

@@ -173,15 +173,15 @@ impl SpillMetrics {
 #[derive(Debug, Clone)]
 pub struct SplitMetrics {
    /// Number of times an input [`RecordBatch`] was split
-    pub batches_splitted: Count,


This looks like an api change

ahh yes. Do we need to extract a dedicated PR for breaking changes? (this one and the ignored Serializeable)

i don't think we need a dedicated PR. We could add a note in the upgrade guide.

Given that these are such small changes, maybe just adding the api-change label so they are highlighted in the release notes would be good enough

Sounds good, I also added a note to PR description 👍

datafusion/proto/tests/cases/roundtrip_physical_plan.rs

datafusion/sql/src/expr/function.rs

findepi · 2025-08-13T09:34:07Z

datafusion/sql/tests/sql_integration.rs

+#[case::missing_assignment_target("UPDATE person SET doesnotexist = true")]
+#[case::missing_assignment_expression("UPDATE person SET age = doesnotexist + 42")]


This was clearly a typo. In rstest? In our code? Did it work, or should we just delete these lines?

I would feel better if this change is backed out from this PR, because I don't understand its consequences

Suggested change

#[case::missing_assignment_target("UPDATE person SET doesnotexist = true")]

#[case::missing_assignment_expression("UPDATE person SET age = doesnotexist + 42")]

#[case::missing_assignement_target("UPDATE person SET doesnotexist = true")]

#[case::missing_assignement_expression("UPDATE person SET age = doesnotexist + 42")]

This is from our side. These strings will become a test case name.

findepi · 2025-08-13T09:36:40Z

typos.toml

+nd = "nd"
+Nd = "Nd"
+YOUY = "YOUY"
+typ = "typ"


this is perhaps fine as type is reserved. worth adding a comment line that typ stands for type

findepi · 2025-08-13T09:36:45Z

typos.toml

+numer = "numer"
+abd = "abd"
+aroun = "aroun"
+carefull = "carefull"


most of these words are either typos, or some arbitrary abbreviations.

Let's not add this file in this PR.
Let's add it in a PR that adds the checking workflow.

findepi · 2025-08-13T09:37:57Z

typos.toml

+alph = "alph"
+wih = "wih"
+Ded = "Ded"
+Serializeable = "Serializeable"


Does the checker have a way to exclude given work at given use place? or only global excludes are available?

alamb · 2025-08-15T20:35:45Z

Is this one ready to merge? It looks like there are some conflicts to resolve and some unresolved comments

waynexia · 2025-08-18T05:09:18Z

Yes it's close. I'll address conflicts and comments tomorrow. Got some issues connecting to my server now 🥲

Signed-off-by: Ruihang Xia <[email protected]>

Co-authored-by: Piotr Findeisen <[email protected]>

Signed-off-by: Ruihang Xia <[email protected]>

waynexia added 10 commits August 8, 2025 08:18

ci: add typo checker

2911068

Signed-off-by: Ruihang Xia <[email protected]>

ignore some files

4ce58bd

Signed-off-by: Ruihang Xia <[email protected]>

ignore changelog

51718dc

Signed-off-by: Ruihang Xia <[email protected]>

trivial typo fix

d3e8073

Signed-off-by: Ruihang Xia <[email protected]>

fix test name

a87250f

Signed-off-by: Ruihang Xia <[email protected]>

fix some

183a862

Signed-off-by: Ruihang Xia <[email protected]>

fix some

79d82bc

Signed-off-by: Ruihang Xia <[email protected]>

update rule

8ca699f

Signed-off-by: Ruihang Xia <[email protected]>

fix some

6d620ef

Signed-off-by: Ruihang Xia <[email protected]>

fix some

fffef95

Signed-off-by: Ruihang Xia <[email protected]>

waynexia commented Aug 12, 2025

View reviewed changes

waynexia mentioned this pull request Aug 12, 2025

speedup date_trunc (~7x faster) in some cases #16859

Merged

waynexia added 2 commits August 12, 2025 11:10

Merge branch 'main' into typo-checker

79f494a

Signed-off-by: Ruihang Xia <[email protected]>

fix main

684e43d

Signed-off-by: Ruihang Xia <[email protected]>

waynexia commented Aug 12, 2025

View reviewed changes

waynexia force-pushed the typo-checker branch from 0a82c6b to 684e43d Compare August 12, 2025 03:27

zhuqi-lucas approved these changes Aug 12, 2025

View reviewed changes

findepi reviewed Aug 12, 2025

View reviewed changes

remove pipeline job from this PR

e63f74c

Signed-off-by: Ruihang Xia <[email protected]>

findepi changed the title ~~ci: add typo checker~~ chore: fix typos Aug 13, 2025

findepi reviewed Aug 13, 2025

View reviewed changes

findepi added the api change Changes the API exposed to users of the crate label Aug 13, 2025

waynexia and others added 6 commits August 19, 2025 02:32

Merge branch 'main' into typo-checker

a85190d

Signed-off-by: Ruihang Xia <[email protected]>

remove typos.toml

172eca8

Signed-off-by: Ruihang Xia <[email protected]>

revert data_vec

ea7ea96

Signed-off-by: Ruihang Xia <[email protected]>

Update datafusion/proto/tests/cases/roundtrip_physical_plan.rs

622442f

Co-authored-by: Piotr Findeisen <[email protected]>

Update datafusion/sql/src/expr/function.rs

b749d0d

Co-authored-by: Piotr Findeisen <[email protected]>

fix compiles

5138386

Signed-off-by: Ruihang Xia <[email protected]>

		#[case::missing_assignment_target("UPDATE person SET doesnotexist = true")]
		#[case::missing_assignment_expression("UPDATE person SET age = doesnotexist + 42")]

chore: fix typos #17135

Are you sure you want to change the base?

chore: fix typos #17135

Uh oh!

Conversation

waynexia commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuqi-lucas left a comment

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waynexia commented Aug 12, 2025

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 15, 2025

Uh oh!

waynexia commented Aug 18, 2025

Uh oh!

Uh oh!

waynexia commented Aug 12, 2025 •

edited

Loading