Skip to content

Commit 875d1dd

Browse files
emanzxclaude
andcommitted
fix(pgwire): preserve declared integer width in RowDescription OIDs
Schemaless/columnar collections resolve every integer width (SMALLINT, INT4, BIGINT, ...) to the single planner-facing `SqlDataType::Int64`, and `sql_data_type_to_ddl_col_type` mapped that straight to OID 20 (int8) for every column regardless of what was actually declared. Clients that declared `SMALLINT` or `INT4` got back OID 20 instead of the expected 21/23, which breaks ORMs and typed client libraries that trust the advertised wire OID. `SMALLINT`/`INT2` were also missing from the schemaless type-string parser and fell through to a `String` default, rendering as OID 25 (text) instead of an integer OID at all. The catalog now threads the column's raw declared type string through to the planner alongside its resolved `SqlDataType`, and a new `sql_data_type_to_ddl_col_type_with_raw` uses it to narrow an `Int8` result to `Int4`/`Int2` when the raw type says so. Only the wire-advertised width changes; storage stays a full i64 either way. Fixes #217. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193ftf9LuUWejMqe2KVWkya
1 parent 79a5f0c commit 875d1dd

5 files changed

Lines changed: 252 additions & 8 deletions

File tree

nodedb/src/control/planner/catalog_adapter/type_convert.rs

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,12 @@ pub(super) fn convert_collection_type(
6161
nullable: true,
6262
is_primary_key: false,
6363
default: None,
64-
raw_type: None,
64+
// Mirrors the columnar branch below: the raw declared
65+
// type string is the only place the wire-width refiner
66+
// (`sql_data_type_to_ddl_col_type_with_raw`) can recover
67+
// `SMALLINT`/`INT4` vs `BIGINT` — `parse_type_str`
68+
// collapses all of them to the same `SqlDataType::Int64`.
69+
raw_type: Some(type_str.clone()),
6570
});
6671
}
6772
(EngineType::DocumentSchemaless, columns, Some(pk_name))
@@ -180,7 +185,7 @@ fn parse_type_str(s: &str) -> SqlDataType {
180185
return SqlDataType::Decimal;
181186
}
182187
match upper.as_str() {
183-
"INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" => SqlDataType::Int64,
188+
"INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" | "SMALLINT" | "INT2" => SqlDataType::Int64,
184189
"FLOAT" | "FLOAT4" | "FLOAT8" | "FLOAT64" | "DOUBLE" | "REAL" => SqlDataType::Float64,
185190
"BOOL" | "BOOLEAN" => SqlDataType::Bool,
186191
"BYTES" | "BYTEA" | "BLOB" => SqlDataType::Bytes,
@@ -193,9 +198,23 @@ fn parse_type_str(s: &str) -> SqlDataType {
193198
mod tests {
194199
use nodedb_types::CollectionType;
195200

196-
use super::{SqlDataType, convert_collection_type};
201+
use super::{SqlDataType, convert_collection_type, parse_type_str};
197202
use crate::control::security::catalog::StoredCollection;
198203

204+
/// `SMALLINT`/`INT2` are valid PostgreSQL wire-width integer keywords
205+
/// (issue #217) that must resolve to the same `SqlDataType::Int64` arm as
206+
/// `INT`/`INTEGER`/`INT4`/`INT8`/`BIGINT` — previously they were unlisted
207+
/// and fell through to the `_ => SqlDataType::String` default, which is
208+
/// what produced the wire OID 25 (text) bug for `SMALLINT` columns.
209+
#[test]
210+
fn parse_type_str_smallint_and_int2_map_to_int64() {
211+
assert_eq!(parse_type_str("SMALLINT"), SqlDataType::Int64);
212+
assert_eq!(parse_type_str("INT2"), SqlDataType::Int64);
213+
// Case-insensitivity, matching every other arm in this function.
214+
assert_eq!(parse_type_str("smallint"), SqlDataType::Int64);
215+
assert_eq!(parse_type_str("int2"), SqlDataType::Int64);
216+
}
217+
199218
/// A columnar (or spatial, which shares the same non-timeseries
200219
/// synthetic-PK path) collection whose DDL declares an explicit
201220
/// `id` field must not surface two `id` columns to the planner —

nodedb/src/control/planner/sql_plan_convert/output_schema.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use nodedb_sql::types_expr::SqlExpr;
2020
use super::lateral::collection_name_from_plan;
2121
use super::output_schema_types::{infer_aggregate_type, infer_computed_expr_type};
2222
use crate::control::server::response_shape::schema::{
23-
OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type,
23+
OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type_with_raw,
2424
};
2525
use crate::control::server::response_shape::types::DdlColType;
2626

@@ -94,7 +94,12 @@ fn column_types_for<C: SqlCatalog>(
9494
Ok(Some(info)) => info
9595
.columns
9696
.iter()
97-
.map(|c| (c.name.clone(), sql_data_type_to_ddl_col_type(&c.data_type)))
97+
.map(|c| {
98+
(
99+
c.name.clone(),
100+
sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()),
101+
)
102+
})
98103
.collect(),
99104
_ => HashMap::new(),
100105
}
@@ -173,7 +178,7 @@ fn ordered_columns_for<C: SqlCatalog>(
173178
.map(|c| OutputColumn {
174179
display_name: c.name.clone(),
175180
lookup_key: c.name.clone(),
176-
ty: sql_data_type_to_ddl_col_type(&c.data_type),
181+
ty: sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()),
177182
})
178183
.collect(),
179184
_ => Vec::new(),

nodedb/src/control/server/response_shape/schema.rs

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,147 @@ pub fn sql_data_type_to_ddl_col_type(
6363
}
6464
}
6565

66+
/// Like [`sql_data_type_to_ddl_col_type`], but refines an `Int8` result using
67+
/// the catalog's declared raw type string (`ColumnInfo::raw_type`), when
68+
/// present.
69+
///
70+
/// # Why only `Int8` is refined
71+
///
72+
/// `SqlDataType::Int64` is the single planner-facing type for every integer
73+
/// width — `SMALLINT`/`INT2`, `INTEGER`/`INT4`, and `BIGINT`/`INT8` all parse
74+
/// to it (see `catalog_adapter::type_convert::parse_type_str` and
75+
/// `nodedb_types::columnar::ColumnType::from_str`), because nodedb's
76+
/// storage — columnar, strict, and kv alike — always keeps integers as a
77+
/// full `i64`. The declared width therefore carries no storage or
78+
/// computation meaning; it is authoritative for exactly one thing: the wire
79+
/// type a pgwire client expects in `RowDescription`. A client that declared
80+
/// `SMALLINT` expects OID 21 (and, for binary-format results, a 2-byte
81+
/// value) — silently widening every integer to `BIGINT`'s OID 20 breaks ORMs
82+
/// and typed client libraries that trust the advertised OID (nodedb upstream
83+
/// issue #217). So this function narrows *display width only*, strictly
84+
/// downstream of `sql_data_type_to_ddl_col_type`'s own type mapping: every
85+
/// other `SqlDataType` variant (`String`, `Float64`, `Bool`, ...) passes
86+
/// through unchanged, with `raw` ignored, because there is no other
87+
/// wire-ambiguous case to resolve.
88+
///
89+
/// # Matching
90+
///
91+
/// `raw` is matched case-insensitively by keyword, using the same
92+
/// prefix-with-boundary logic as
93+
/// `pgwire::catalog::tables::collections::field_type_to_oid` (the
94+
/// already-correct reference implementation this mirrors): `BIGINT`/`INT8`
95+
/// are checked *first* so a plain `INT` prefix check can't accidentally
96+
/// swallow `INT8` (there is no separator between `INT` and `8` for the
97+
/// boundary check to reject). An unrecognized or absent `raw` leaves the
98+
/// base `Int8` untouched — refinement is best-effort, never a hard error.
99+
pub fn sql_data_type_to_ddl_col_type_with_raw(
100+
ty: &nodedb_sql::types_expr::SqlDataType,
101+
raw: Option<&str>,
102+
) -> super::types::DdlColType {
103+
use super::types::DdlColType;
104+
105+
let base = sql_data_type_to_ddl_col_type(ty);
106+
let (DdlColType::Int8, Some(raw)) = (base, raw) else {
107+
return base;
108+
};
109+
110+
let normalized = raw.trim().to_ascii_lowercase();
111+
let starts_with_type = |name: &str| {
112+
normalized == name
113+
|| normalized.strip_prefix(name).is_some_and(|rest| {
114+
rest.starts_with('(') || rest.chars().next().is_some_and(char::is_whitespace)
115+
})
116+
};
117+
118+
if starts_with_type("bigint") || starts_with_type("int8") {
119+
DdlColType::Int8
120+
} else if starts_with_type("smallint") || starts_with_type("int2") {
121+
DdlColType::Int2
122+
} else if starts_with_type("integer") || starts_with_type("int4") || starts_with_type("int") {
123+
DdlColType::Int4
124+
} else {
125+
DdlColType::Int8
126+
}
127+
}
128+
66129
#[cfg(test)]
67130
mod tests {
68131
use super::super::types::DdlColType;
69132
use super::*;
70133
use nodedb_sql::types_expr::SqlDataType;
71134

135+
/// `sql_data_type_to_ddl_col_type_with_raw` must narrow an `Int64`
136+
/// (base `Int8`) column to `Int4`/`Int2` when the catalog's declared raw
137+
/// type string says the DDL author wrote a narrower integer keyword.
138+
/// `BIGINT`/`INT8` stay `Int8`. Case-insensitive, matching every other
139+
/// raw-type match in this codebase (e.g. `field_type_to_oid`).
140+
#[test]
141+
fn refine_narrows_int8_by_raw_integer_keyword() {
142+
let cases: &[(&str, DdlColType)] = &[
143+
("BIGINT", DdlColType::Int8),
144+
("INT8", DdlColType::Int8),
145+
("INTEGER", DdlColType::Int4),
146+
("INT", DdlColType::Int4),
147+
("INT4", DdlColType::Int4),
148+
("SMALLINT", DdlColType::Int2),
149+
("INT2", DdlColType::Int2),
150+
];
151+
for (raw, expected) in cases {
152+
assert_eq!(
153+
sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some(raw)),
154+
*expected,
155+
"raw type {raw:?} must refine to {expected:?}"
156+
);
157+
// Case-insensitivity.
158+
assert_eq!(
159+
sql_data_type_to_ddl_col_type_with_raw(
160+
&SqlDataType::Int64,
161+
Some(&raw.to_lowercase())
162+
),
163+
*expected,
164+
"lowercase raw type {raw:?} must refine to {expected:?}"
165+
);
166+
}
167+
}
168+
169+
/// A raw string that isn't a recognized integer keyword leaves the base
170+
/// `Int8` untouched — the refinement is best-effort, never a hard error.
171+
#[test]
172+
fn refine_unrecognized_raw_keyword_stays_int8() {
173+
assert_eq!(
174+
sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some("NOT_A_TYPE")),
175+
DdlColType::Int8
176+
);
177+
}
178+
179+
/// `raw = None` (planner-synthesized columns, e.g. an auto-injected
180+
/// `id`, carry no raw type) passes through to the unrefined base type.
181+
#[test]
182+
fn refine_with_no_raw_type_stays_base() {
183+
assert_eq!(
184+
sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, None),
185+
DdlColType::Int8
186+
);
187+
}
188+
189+
/// Only an `Int8` base is eligible for refinement — every other
190+
/// `SqlDataType` passes through `sql_data_type_to_ddl_col_type` exactly,
191+
/// with `raw` ignored even when it names an integer keyword (a raw type
192+
/// string can never disagree with the planner's own resolved
193+
/// `SqlDataType`, but this proves the fn doesn't misapply integer
194+
/// narrowing to non-integer columns regardless).
195+
#[test]
196+
fn refine_passes_through_non_int8_types_untouched() {
197+
assert_eq!(
198+
sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::String, Some("SMALLINT")),
199+
DdlColType::Text
200+
);
201+
assert_eq!(
202+
sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Float64, None),
203+
DdlColType::Float8
204+
);
205+
}
206+
72207
#[test]
73208
fn maps_every_sql_data_type_variant() {
74209
assert_eq!(

nodedb/tests/pgwire_ddl_result_types.rs

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ use tokio::net::TcpStream;
2727
/// PostgreSQL built-in type OIDs (stable, wire-level constants).
2828
const OID_TEXT: u32 = 25;
2929
const OID_INT8: u32 = 20;
30+
const OID_INT4: u32 = 23;
31+
const OID_INT2: u32 = 21;
3032

3133
/// A decoded simple-query result: the `RowDescription` columns as
3234
/// `(name, type_oid)` pairs, plus each `DataRow`'s fields as raw text
@@ -262,3 +264,83 @@ async fn show_collections_row_description_preserves_int8_column() {
262264
"created_at INT8 value must round-trip as a decimal integer, got {created_at:?}"
263265
);
264266
}
267+
268+
/// Upstream issue #217 repro: integer OID wire fidelity. A schemaless
269+
/// `CREATE COLLECTION` (no `WITH (engine=...)` clause) declaring every
270+
/// PostgreSQL integer width — `INT`, `INTEGER`, `INT4`, `BIGINT`, `INT8`,
271+
/// `SMALLINT`, `INT2` — must advertise each column's *own* wire OID in
272+
/// `RowDescription`: 23 (int4) for the 4-byte aliases, 20 (int8) for the
273+
/// 8-byte aliases, 21 (int2) for the 2-byte aliases. Before the fix, every
274+
/// declared width collapsed to `SqlDataType::Int64`'s single wire mapping —
275+
/// `INT`/`INTEGER`/`INT4`/`BIGINT`/`INT8` all rendered as OID 20 (int8), and
276+
/// `SMALLINT`/`INT2` (unlisted in the type-string parser) fell through to
277+
/// the `String` default and rendered as OID 25 (text) instead of an integer
278+
/// OID at all.
279+
#[tokio::test]
280+
async fn create_collection_int_widths_preserve_wire_oids() {
281+
let server = TestServer::start().await;
282+
server
283+
.exec(
284+
"CREATE COLLECTION fixprobe_ints (\
285+
id TEXT PRIMARY KEY, \
286+
a INT, \
287+
b INTEGER, \
288+
c INT4, \
289+
d BIGINT, \
290+
e INT8, \
291+
f SMALLINT, \
292+
g INT2\
293+
)",
294+
)
295+
.await
296+
.expect("CREATE COLLECTION fixprobe_ints must succeed");
297+
server
298+
.exec(
299+
"INSERT INTO fixprobe_ints (id, a, b, c, d, e, f, g) \
300+
VALUES ('r1', 1, 2, 3, 4, 5, 6, 7)",
301+
)
302+
.await
303+
.expect("INSERT fixprobe_ints must succeed");
304+
305+
let res = raw_simple_query(
306+
server.pg_port,
307+
"SELECT id, a, b, c, d, e, f, g FROM fixprobe_ints",
308+
)
309+
.await;
310+
311+
let expected = vec![
312+
("id".to_string(), OID_TEXT),
313+
("a".to_string(), OID_INT4),
314+
("b".to_string(), OID_INT4),
315+
("c".to_string(), OID_INT4),
316+
("d".to_string(), OID_INT8),
317+
("e".to_string(), OID_INT8),
318+
("f".to_string(), OID_INT2),
319+
("g".to_string(), OID_INT2),
320+
];
321+
assert_eq!(
322+
res.columns, expected,
323+
"fixprobe_ints RowDescription (name, OID) must preserve each declared \
324+
integer width — INT/INTEGER/INT4 -> int4 (23), BIGINT/INT8 -> int8 (20), \
325+
SMALLINT/INT2 -> int2 (21)"
326+
);
327+
328+
let row = res
329+
.rows
330+
.first()
331+
.unwrap_or_else(|| panic!("expected 1 row, got: {:?}", res.rows));
332+
assert_eq!(
333+
row,
334+
&vec![
335+
Some("r1".to_string()),
336+
Some("1".to_string()),
337+
Some("2".to_string()),
338+
Some("3".to_string()),
339+
Some("4".to_string()),
340+
Some("5".to_string()),
341+
Some("6".to_string()),
342+
Some("7".to_string()),
343+
],
344+
"fixprobe_ints row values must round-trip unchanged"
345+
);
346+
}

nodedb/tests/pgwire_extended_query_engines.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,10 +194,13 @@ async fn extended_query_columnar_typed_scan() {
194194
let id: &str = rows[0].get("id");
195195
assert_eq!(id, "r1");
196196

197-
// RowDescription: id→TEXT(25), n→INT8(20), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25).
197+
// RowDescription: id→TEXT(25), n→INT4(23), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25).
198+
// `n` is declared `INT` in the DDL above, which is the INT4 alias — issue
199+
// #217 fixed this column advertising OID 20 (int8) for every declared
200+
// integer width; it now correctly narrows to OID 23 (int4).
198201
let expected: &[(&str, u32)] = &[
199202
("id", 25),
200-
("n", 20),
203+
("n", 23),
201204
("score", 701),
202205
("flag", 16),
203206
("label", 25),

0 commit comments

Comments
 (0)