Skip to content

Conversation

@Lege19
Copy link

@Lege19 Lege19 commented Nov 5, 2025

Does your PR solve an issue?

fixes #3967

Is this a breaking change?

This is a breaking change.
Previously an INTEGER PRIMARY KEY column was wrongly inferred to be nullable, so code using query! will break, as the result will now be an i64 instead of an Option<i64>.
query_as! should be fine though because Option<i64> implements From<i64>

This will apply anywhere someone has used an INTEGER PRIMARY KEY and not overridden the nullability of that column in their queries.

Comment on lines 1824 to 1831
assert!(
if let Ok((ty, nullable)) = explain(&mut conn, "SELECT * FROM not_an_alias") {
ty.as_slice() == &[SqliteTypeInfo(DataType::Integer)]
&& nullable.as_slice() == &[Some(true)]
} else {
false
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It might be worth an additional assertion for selecting rowid directly from each of the tables, since rowid behavior is unchanged between the 2 tables.

@tyrelr
Copy link
Contributor

tyrelr commented Nov 15, 2025

The explain.rs changes look reasonable to me. But I haven't touched sqlx in a long time, so I won't try to guess whether returning None from handle.rs column_nullable could lead to surprises.

I initially assumed that the handle.rs is not needed for the macro magic to work. But I vaguely remember some portion of the query type describe mechanism executed select queries to sniff the results... so maybe that is what forced the handle.rs changes?

@Lege19
Copy link
Author

Lege19 commented Nov 15, 2025

column_nullable is only called from sqlx-sqlite/src/connection/describe.rs in function describe.
In that function the explain.rs nullability is used as a fallback.
I couldn't think of a reasonably efficient way to determine if a column was a rowid alias in column_nullable. However in explain.rs, obviously SQLite knows which columns are rowid aliases, and it uses special opcodes in those cases which are easy to recognise and handle.

The requested extra test makes sense. Will add.

Tests the nullability of explicitly referring to the rowid column (not
through an alias).
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.

query! macro infers type as Option for primary key

2 participants