Skip to content

Conversation

@rubenfiszel
Copy link

We got quite burned by this for queries where id() would just panic.
Example of a query that would panic:

select Date,Env from read_csv($file);

with csv:

Date,Env
"foo","bar"

and us running:

                            (0..stmt.column_count())
                                .map(|i| {
                                    let logical_type = stmt.column_logical_type(i);
                                    if logical_type.is_invalid() {
                                        None
                                    } else {
                                        logical_type.get_alias()
                                    }
                                })
                                .collect::<Vec<

on first row (note that i'm using is_invalid here and otherwise we would have fetched get_alias directly and got a panic

@diegoimbert
Copy link
Contributor

diegoimbert commented Nov 15, 2025

I'd add that I think that the real problem is that invalid logical type id shouldn't be returned from that query, but it's still nice to be able to check if the id is valid for worst case scenario

@mlafeldt
Copy link
Member

Thanks for the PR!

I took a look at the underlying problem and think that #629 would be a better fix that also addresses your particular needs. Do you agree?

@rubenfiszel
Copy link
Author

Yes I agree that would also work but I would still argue that the semantic of having panic there seems wrong. It should be an unreachable! if it's truly unreachable after that change.

@mlafeldt
Copy link
Member

mlafeldt commented Nov 20, 2025

Good point. I changed LogicalTypeId to be non_exhaustive and return a new "unsupported" value. There's also a new try_id helper.

PTAL: #629

mlafeldt added a commit that referenced this pull request Nov 20, 2025
@mlafeldt mlafeldt closed this Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants