Skip to content

Conversation

@emilk
Copy link
Contributor

@emilk emilk commented Jan 23, 2026

Rationale for this change

I believe that error messages should be as readable as possible. Aim for rustc more than gcc.

Display is the nice, user-facing formatter. Debug is for… well, debugging.

What changes are included in this PR?

Change a bunch of {:?} format string to {}. I'm sure I missed a lot of them, because I know of no way to enforce this without

Are these changes tested?

I assume CI runs cargo test :)

Are there any user-facing changes?

Yes! Error messages should be a bit more readable now.

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate functions Changes to functions implementation datasource Changes to the datasource crate spark labels Jan 23, 2026
@emilk emilk changed the title Improve error message Improve error messages Jan 23, 2026
@timsaucer timsaucer self-requested a review January 23, 2026 11:45
@timsaucer
Copy link
Member

Looks great, just a few easy CI errors.LMK if you want me to knock them out.

@emilk
Copy link
Contributor Author

emilk commented Jan 23, 2026

That would be appreciated ❤️

@emilk
Copy link
Contributor Author

emilk commented Jan 23, 2026

Btw, datafusion currently uses ;N suffix for "nullable", while arrow-rs now uses an nullable prefix instead: Int32;N vs nullable Int32

Maybe we want to follow arrow-rs here?

(…in another PR)

@github-actions github-actions bot added the optimizer Optimizer rules label Jan 23, 2026
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.

Looks good to me too --thanks @emilk and @timsaucer

SELECT
-- nanoseconds, with no, utc, and local timezone
arrow_cast(column1, 'Timestamp(Nanosecond, None)') as ts_nano_no_tz,
arrow_cast(column1, 'Timestamp(ns)') as ts_nano_no_tz,
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 it is valuable to have at least a few tests that verify the old syntax still works, FWIW

Copy link
Member

Choose a reason for hiding this comment

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

Good call, added.

Projection: shapes.shape_id [shape_id:UInt32]
Unnest: lists[shape_id2|depth=1] structs[] [shape_id:UInt32, shape_id2:UInt32;N]
Aggregate: groupBy=[[shapes.shape_id]], aggr=[[array_agg(shapes.shape_id) AS shape_id2]] [shape_id:UInt32, shape_id2:List(Field { data_type: UInt32, nullable: true });N]
Aggregate: groupBy=[[shapes.shape_id]], aggr=[[array_agg(shapes.shape_id) AS shape_id2]] [shape_id:UInt32, shape_id2:List(UInt32);N]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb alamb changed the title Improve error messages Improve error messages with nicer formatting of Date and Time types Jan 23, 2026
@timsaucer timsaucer added this pull request to the merge queue Jan 24, 2026
Merged via the queue into apache:main with commit c135236 Jan 24, 2026
32 checks passed
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 datasource Changes to the datasource crate functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates spark sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants