Skip to content

Commit 0f06e64

Browse files
authored
Merge pull request #224 from emanzx/fix/217-integer-oid-fidelity
fix(pgwire): preserve declared integer width in RowDescription OIDs; accept INT4/INT8/SMALLINT/INT2 in strict DDL
2 parents 34fe6a3 + 1d2c82b commit 0f06e64

28 files changed

Lines changed: 1480 additions & 29 deletions

nodedb-sql/src/error.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,21 @@ pub enum SqlError {
2020
#[error("type mismatch: {detail}")]
2121
TypeMismatch { detail: String },
2222

23+
/// A write supplied an integer wider than the column's declared type.
24+
///
25+
/// nodedb stores every integer as an `i64`, so this is not a storage
26+
/// limit — it is the constraint that makes the column's advertised wire
27+
/// type honest. A column declared `INTEGER` reports OID 23, and a pgwire
28+
/// client reading it in binary format decodes exactly four bytes; letting
29+
/// a wider value in would mean either truncating it on read or lying about
30+
/// the type. PostgreSQL rejects the same write with the same message.
31+
#[error("value {value} is out of range for column '{column}' of type {declared_type}")]
32+
IntegerOutOfRange {
33+
column: String,
34+
value: i64,
35+
declared_type: &'static str,
36+
},
37+
2338
#[error("unsupported: {detail}")]
2439
Unsupported { detail: String },
2540

nodedb-sql/src/planner/dml.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ use nodedb_types::DatabaseId;
66
use sqlparser::ast::{self};
77

88
use super::dml_helpers::{
9-
build_kv_insert_plan, build_vector_primary_insert_plan, convert_value_rows,
10-
resolve_insert_columns,
9+
build_kv_insert_plan, build_vector_primary_insert_plan, check_declared_int_ranges,
10+
convert_value_rows, resolve_insert_columns,
1111
};
1212
use crate::engine_rules::{self, InsertParams};
1313
use crate::error::{Result, SqlError};
@@ -135,6 +135,7 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
135135
intent,
136136
Vec::new(),
137137
info.primary_key.as_deref(),
138+
&info.columns,
138139
);
139140
}
140141

@@ -148,11 +149,13 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
148149
&& let Some(ref vpc) = info.vector_primary
149150
{
150151
let rows_parsed = convert_value_rows(&columns, rows_ast)?;
152+
check_declared_int_ranges(&info.columns, &rows_parsed)?;
151153
return build_vector_primary_insert_plan(&table_name, vpc, &columns, rows_parsed);
152154
}
153155

154156
// All other engines: delegate to engine rules.
155157
let rows = convert_value_rows(&columns, rows_ast)?;
158+
check_declared_int_ranges(&info.columns, &rows)?;
156159
let column_defaults: Vec<(String, String)> = info
157160
.columns
158161
.iter()
@@ -218,6 +221,7 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
218221
KvInsertIntent::Put,
219222
Vec::new(),
220223
info.primary_key.as_deref(),
224+
&info.columns,
221225
);
222226
}
223227

@@ -226,6 +230,7 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
226230
let columns = resolve_insert_columns(columns, &info, rows_ast)?;
227231

228232
let rows = convert_value_rows(&columns, rows_ast)?;
233+
check_declared_int_ranges(&info.columns, &rows)?;
229234
let column_defaults: Vec<(String, String)> = info
230235
.columns
231236
.iter()
@@ -294,6 +299,7 @@ fn plan_upsert_with_on_conflict(
294299
KvInsertIntent::Put,
295300
on_conflict_updates,
296301
info.primary_key.as_deref(),
302+
&info.columns,
297303
);
298304
}
299305

@@ -302,6 +308,7 @@ fn plan_upsert_with_on_conflict(
302308
let columns = resolve_insert_columns(columns, &info, rows_ast)?;
303309

304310
let rows = convert_value_rows(&columns, rows_ast)?;
311+
check_declared_int_ranges(&info.columns, &rows)?;
305312
let column_defaults: Vec<(String, String)> = info
306313
.columns
307314
.iter()

nodedb-sql/src/planner/dml_helpers.rs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,85 @@ pub(super) fn convert_value_rows(
2525
.collect()
2626
}
2727

28+
/// Reject any integer value that does not fit its column's declared width.
29+
///
30+
/// nodedb stores every integer as an `i64`, so this is not a storage limit.
31+
/// It is the constraint that makes the column's advertised wire type honest:
32+
/// a column declared `INTEGER` reports OID 23 in `RowDescription`, and a
33+
/// pgwire client reading it in binary format decodes exactly four bytes.
34+
/// Accepting a wider value would force a later choice between truncating it on
35+
/// read and lying about the column's type — so the value is refused at the
36+
/// point it enters, exactly as PostgreSQL refuses it.
37+
///
38+
/// This runs in the planner rather than in each engine because the declared
39+
/// width is engine-independent (the same `IntWidth` drives the wire type for
40+
/// schemaless, columnar, strict, and kv alike), and because parameters are
41+
/// bound into the AST before planning — so one check here covers both literal
42+
/// `VALUES` and `$1` placeholders, for every engine, on every DML path.
43+
///
44+
/// Non-integer values and columns with no declared width pass through: this
45+
/// checks range only, never type.
46+
pub(super) fn check_declared_int_ranges(
47+
columns: &[ColumnInfo],
48+
rows: &[Vec<(String, SqlValue)>],
49+
) -> Result<()> {
50+
// Overwhelmingly the common case — skip the per-cell name lookup entirely
51+
// when the collection declares no narrowed integer column.
52+
if !columns.iter().any(|c| {
53+
matches!(
54+
c.int_width,
55+
Some(nodedb_types::columnar::IntWidth::I16 | nodedb_types::columnar::IntWidth::I32)
56+
)
57+
}) {
58+
return Ok(());
59+
}
60+
61+
for row in rows {
62+
for (name, value) in row {
63+
let SqlValue::Int(v) = value else { continue };
64+
let Some(width) = columns
65+
.iter()
66+
.find(|c| c.name.eq_ignore_ascii_case(name))
67+
.and_then(|c| c.int_width)
68+
else {
69+
continue;
70+
};
71+
if !width.contains(*v) {
72+
return Err(SqlError::IntegerOutOfRange {
73+
column: name.clone(),
74+
value: *v,
75+
declared_type: width.pg_type_name(),
76+
});
77+
}
78+
}
79+
}
80+
Ok(())
81+
}
82+
83+
/// [`check_declared_int_ranges`] for `UPDATE ... SET col = <literal>`.
84+
///
85+
/// Only literal assignments are checkable at plan time; a computed assignment
86+
/// (`SET n = n + 1`) has no value until the Data Plane evaluates it. Those are
87+
/// caught on the read path instead, where the encoder refuses to transmit a
88+
/// value that does not fit the column's advertised width — so an out-of-range
89+
/// value can never reach a client silently by either route.
90+
pub(super) fn check_declared_int_ranges_in_assignments(
91+
columns: &[ColumnInfo],
92+
assignments: &[(String, SqlExpr)],
93+
) -> Result<()> {
94+
let literals: Vec<(String, SqlValue)> = assignments
95+
.iter()
96+
.filter_map(|(col, expr)| match expr {
97+
SqlExpr::Literal(v @ SqlValue::Int(_)) => Some((col.clone(), v.clone())),
98+
_ => None,
99+
})
100+
.collect();
101+
if literals.is_empty() {
102+
return Ok(());
103+
}
104+
check_declared_int_ranges(columns, std::slice::from_ref(&literals))
105+
}
106+
28107
/// Resolve the effective column list for a `VALUES`-clause INSERT/UPSERT.
29108
///
30109
/// A *positional* insert — `INSERT INTO t VALUES (...)` with no explicit
@@ -277,6 +356,7 @@ pub(super) fn build_kv_insert_plan(
277356
intent: KvInsertIntent,
278357
on_conflict_updates: Vec<(String, SqlExpr)>,
279358
pk_col: Option<&str>,
359+
declared_columns: &[ColumnInfo],
280360
) -> Result<Vec<SqlPlan>> {
281361
// Positional KV insert (no column list): the key/value split below is
282362
// driven entirely by matching column *names* against `key_col_name`/
@@ -288,6 +368,10 @@ pub(super) fn build_kv_insert_plan(
288368
collection: table_name,
289369
});
290370
}
371+
// KV returns early from every INSERT/UPSERT entry point, so the declared
372+
// width check lives here rather than at the call sites — otherwise a
373+
// fourth KV entry point could be added without it.
374+
check_declared_int_ranges(declared_columns, &convert_value_rows(columns, rows_ast)?)?;
291375
let key_col_name = pk_col.unwrap_or("key");
292376
let key_idx = columns.iter().position(|c| c == key_col_name);
293377
let ttl_idx = columns.iter().position(|c| c == "ttl");

nodedb-sql/src/planner/dml_update_delete.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,10 @@ use sqlparser::ast;
88
use super::super::ast_helpers::{
99
flatten_and_expr, qualified_ident_pair, strip_and_convert_filters,
1010
};
11-
use super::super::dml_helpers::{extract_point_keys, extract_table_name_from_table_with_joins};
11+
use super::super::dml_helpers::{
12+
check_declared_int_ranges_in_assignments, extract_point_keys,
13+
extract_table_name_from_table_with_joins,
14+
};
1215
use crate::engine_rules::{self, DeleteParams, UpdateFromParams, UpdateParams};
1316
use crate::error::{Result, SqlError};
1417
use crate::parser::normalize::{
@@ -41,6 +44,7 @@ pub fn plan_update(stmt: &ast::Statement, catalog: &dyn SqlCatalog) -> Result<Ve
4144
})?;
4245

4346
let assigns = convert_assignments(&update.assignments)?;
47+
check_declared_int_ranges_in_assignments(&info.columns, &assigns)?;
4448

4549
let filters = match &update.selection {
4650
Some(expr) => super::super::select::convert_where_to_filters(expr)?,

nodedb-sql/src/resolver/columns.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec<ColumnInfo> {
285285
is_primary_key: false,
286286
default: None,
287287
raw_type: None,
288+
int_width: None,
288289
});
289290
}
290291
for a in &view.attrs {
@@ -295,6 +296,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec<ColumnInfo> {
295296
is_primary_key: false,
296297
default: None,
297298
raw_type: None,
299+
int_width: None,
298300
});
299301
}
300302
cols
@@ -356,6 +358,7 @@ mod tests {
356358
is_primary_key: false,
357359
default: None,
358360
raw_type: None,
361+
int_width: None,
359362
})
360363
.collect(),
361364
primary_key: None,

nodedb-sql/src/resolver/expr/convert.rs

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
//! Convert sqlparser AST expressions to our SqlExpr IR.
44
5-
use sqlparser::ast::{self, Expr, Value};
5+
use sqlparser::ast::{self, Expr, UnaryOperator, Value};
66

77
use crate::error::{Result, SqlError};
88
use crate::parser::normalize::{SCHEMA_QUALIFIED_MSG, normalize_ident};
@@ -130,6 +130,36 @@ fn convert_expr_inner(expr: &Expr, depth: &mut usize) -> Result<SqlExpr> {
130130
right: Box::new(convert_expr_depth(right, depth)?),
131131
})
132132
}
133+
// A negative integer literal reaches sqlparser as unary minus applied
134+
// to a *positive* number, so the most negative `BIGINT` arrives as
135+
// `-(9223372036854775808)` — and that operand does not fit an `i64`.
136+
// Converting the operand on its own therefore falls back to `Float`
137+
// and silently turns an exact integer into an approximate one. Folding
138+
// the sign into the literal before parsing keeps the whole `i64` range
139+
// exact; anything that still does not fit falls through to the general
140+
// path below and is handled as before.
141+
Expr::UnaryOp {
142+
op: UnaryOperator::Minus,
143+
expr: inner,
144+
} if matches!(
145+
inner.as_ref(),
146+
Expr::Value(v) if matches!(&v.value, Value::Number(..))
147+
) =>
148+
{
149+
let Expr::Value(v) = inner.as_ref() else {
150+
unreachable!("guarded by the `matches!` above")
151+
};
152+
let Value::Number(n, _) = &v.value else {
153+
unreachable!("guarded by the `matches!` above")
154+
};
155+
match format!("-{n}").parse::<i64>() {
156+
Ok(i) => Ok(SqlExpr::Literal(SqlValue::Int(i))),
157+
Err(_) => Ok(SqlExpr::UnaryOp {
158+
op: UnaryOp::Neg,
159+
expr: Box::new(convert_expr_depth(inner, depth)?),
160+
}),
161+
}
162+
}
133163
Expr::UnaryOp { op, expr } => Ok(SqlExpr::UnaryOp {
134164
op: convert_unary_op(op)?,
135165
expr: Box::new(convert_expr_depth(expr, depth)?),

nodedb-sql/src/types/collection.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,14 @@ pub struct ColumnInfo {
7676
/// Columnar INSERT converters use this to reconstruct the exact `ColumnType`
7777
/// so JSON / Geometry / UUID columns are not incorrectly inferred as String.
7878
pub raw_type: Option<String>,
79+
/// Declared width of an integer column, resolved once from the catalog at
80+
/// adapter-construction time. `None` for non-integer columns and for
81+
/// integer columns whose declared type carried no width.
82+
///
83+
/// This is deliberately a resolved [`IntWidth`] rather than another raw
84+
/// string: it is read on both the write path (range validation) and the
85+
/// read path (`RowDescription` OID and binary payload width), and those
86+
/// two must agree exactly. Resolving once at the catalog boundary is what
87+
/// makes disagreement unrepresentable.
88+
pub int_width: Option<nodedb_types::columnar::IntWidth>,
7989
}

nodedb-sql/tests/positional_insert_column_binding.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ impl SqlCatalog for Catalog {
4040
is_primary_key: true,
4141
default: None,
4242
raw_type: Some("INT".into()),
43+
int_width: Some(nodedb_types::columnar::IntWidth::I32),
4344
},
4445
ColumnInfo {
4546
name: "note".into(),
@@ -48,6 +49,7 @@ impl SqlCatalog for Catalog {
4849
is_primary_key: false,
4950
default: None,
5051
raw_type: Some("TEXT".into()),
52+
int_width: None,
5153
},
5254
],
5355
primary_key: Some("id".into()),

nodedb-types/src/columnar/column_parse.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,17 @@ impl FromStr for ColumnType {
121121
}
122122

123123
match upper.as_str() {
124-
"BIGINT" | "INT64" | "INTEGER" | "INT" => Ok(Self::Int64),
124+
// `INT4`/`INT8`/`SMALLINT`/`INT2` are PostgreSQL wire-width integer
125+
// keywords (issue #223: strict/kv `CREATE COLLECTION` rejected
126+
// them as unknown types even though they're valid aliases). They
127+
// all collapse to the same `Int64` storage variant as
128+
// `BIGINT`/`INTEGER`/`INT` — nodedb always stores integers as a
129+
// full i64. The declared width is carried separately as an
130+
// [`super::IntWidth`], which is what bounds writes and narrows the
131+
// advertised wire OID; it is deliberately not a storage variant.
132+
"BIGINT" | "INT64" | "INTEGER" | "INT" | "INT4" | "INT8" | "SMALLINT" | "INT2" => {
133+
Ok(Self::Int64)
134+
}
125135
"FLOAT64" | "DOUBLE" | "REAL" | "FLOAT" => Ok(Self::Float64),
126136
"TEXT" | "STRING" | "VARCHAR" => Ok(Self::String),
127137
"BOOL" | "BOOLEAN" => Ok(Self::Bool),

nodedb-types/src/columnar/column_type.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,20 @@ mod tests {
286286
assert_eq!("UUID".parse::<ColumnType>().unwrap(), ColumnType::Uuid);
287287
}
288288

289+
/// `INT4`/`INT8`/`SMALLINT`/`INT2` are wire-width DDL keywords (issue
290+
/// #223: strict/kv `CREATE COLLECTION` rejected them as unknown column
291+
/// types even though they're valid PostgreSQL integer aliases). They all
292+
/// map to the same `Int64` storage variant as `BIGINT`/`INTEGER`/`INT` —
293+
/// nodedb's columnar/strict/kv storage always keeps integers as a full
294+
/// i64; only the wire (`DdlColType`) layer narrows the advertised OID.
295+
#[test]
296+
fn parse_int_width_aliases_map_to_int64() {
297+
assert_eq!("INT4".parse::<ColumnType>().unwrap(), ColumnType::Int64);
298+
assert_eq!("INT8".parse::<ColumnType>().unwrap(), ColumnType::Int64);
299+
assert_eq!("SMALLINT".parse::<ColumnType>().unwrap(), ColumnType::Int64);
300+
assert_eq!("INT2".parse::<ColumnType>().unwrap(), ColumnType::Int64);
301+
}
302+
289303
#[test]
290304
fn parse_vector() {
291305
assert_eq!(

0 commit comments

Comments
 (0)