Skip to content

Commit f6fd6d9

Browse files
committed
fix(sql): reject float literals that overflow a declared REAL column
Narrowing an f64 to f32 rounds and is never an error, so writes had no declared-width check for floats at all — including on the KV and document-schemaless engines' untyped write paths and on ON CONFLICT DO UPDATE assignments. A finite value beyond f32's range would silently narrow to Infinity on read instead of being refused on write, unlike the equivalent integer check. Add a FloatOutOfRange error and a check_declared_float_ranges pass (mirroring the integer range check) on both the row insert and ON CONFLICT DO UPDATE assignment paths, and route the existing pgwire narrowing guard's docs to point at it as the new write-time backstop it now layers under.
1 parent ec4748f commit f6fd6d9

11 files changed

Lines changed: 518 additions & 65 deletions

File tree

nodedb-sql/src/error.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,22 @@ pub enum SqlError {
3535
declared_type: &'static str,
3636
},
3737

38+
/// A write supplied a finite float that overflows to infinity when
39+
/// narrowed to the column's declared `REAL` width.
40+
///
41+
/// Narrowing an `f64` to `f32` normally rounds — `1.1` becoming
42+
/// `1.10000002` is correct PostgreSQL `real` behaviour, never an error —
43+
/// so this is not the float mirror of [`SqlError::IntegerOutOfRange`]'s
44+
/// full-range check. The single case rejected is a finite value beyond
45+
/// `f32`'s range, which would otherwise silently become `Infinity`.
46+
/// PostgreSQL rejects the same write with the same message.
47+
#[error("value {value} is out of range for column '{column}' of type {declared_type}")]
48+
FloatOutOfRange {
49+
column: String,
50+
value: f64,
51+
declared_type: &'static str,
52+
},
53+
3854
#[error("unsupported: {detail}")]
3955
Unsupported { detail: String },
4056

nodedb-sql/src/planner/dml.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ 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, coerce_and_check_rows,
10-
convert_value_rows, resolve_insert_columns,
9+
build_kv_insert_plan, build_vector_primary_insert_plan,
10+
check_declared_float_ranges_in_assignments, check_declared_int_ranges_in_assignments,
11+
coerce_and_check_rows, convert_value_rows, resolve_insert_columns,
1112
};
1213
use crate::engine_rules::{self, InsertParams};
1314
use crate::error::{Result, SqlError};
@@ -317,6 +318,8 @@ fn plan_upsert_with_on_conflict(
317318
&mut on_conflict_updates,
318319
info.primary_key.as_deref(),
319320
)?;
321+
check_declared_int_ranges_in_assignments(&info.columns, &on_conflict_updates)?;
322+
check_declared_float_ranges_in_assignments(&info.columns, &on_conflict_updates)?;
320323
let column_defaults: Vec<(String, String)> = info
321324
.columns
322325
.iter()

nodedb-sql/src/planner/dml_helpers/kv_insert.rs

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@
55
66
use sqlparser::ast;
77

8-
use super::range_check::check_declared_int_ranges;
8+
use super::range_check::{
9+
check_declared_float_ranges, check_declared_float_ranges_in_assignments,
10+
check_declared_int_ranges, check_declared_int_ranges_in_assignments,
11+
};
912
use super::value_convert::expr_to_sql_value;
1013
use crate::error::{Result, SqlError};
1114
use crate::planner::declared_type_coerce::{
@@ -91,6 +94,7 @@ pub(crate) fn build_kv_insert_plan(
9194
// values so a literal that only becomes an integer through coercion is
9295
// still range-checked against its declared width.
9396
check_declared_int_ranges(declared_columns, &coerced_rows)?;
97+
check_declared_float_ranges(declared_columns, &coerced_rows)?;
9498
// `ON CONFLICT DO UPDATE SET col = <literal>` writes through the same
9599
// untyped KV path as the inserted row, so its literals need the same
96100
// declared-type coercion.
@@ -99,6 +103,8 @@ pub(crate) fn build_kv_insert_plan(
99103
&mut on_conflict_updates,
100104
Some(key_col_name),
101105
)?;
106+
check_declared_int_ranges_in_assignments(declared_columns, &on_conflict_updates)?;
107+
check_declared_float_ranges_in_assignments(declared_columns, &on_conflict_updates)?;
102108

103109
let mut entries = Vec::with_capacity(coerced_rows.len());
104110
let mut ttl_secs: u64 = 0;
@@ -130,3 +136,109 @@ pub(crate) fn build_kv_insert_plan(
130136
on_conflict_updates,
131137
}])
132138
}
139+
140+
#[cfg(test)]
141+
mod kv_on_conflict_range_tests {
142+
use sqlparser::ast::{Expr, Value, ValueWithSpan};
143+
use sqlparser::tokenizer::Span;
144+
145+
use super::*;
146+
use nodedb_types::columnar::{FloatWidth, IntWidth};
147+
148+
fn string_column(name: &str) -> ColumnInfo {
149+
ColumnInfo {
150+
name: name.to_string(),
151+
data_type: SqlDataType::String,
152+
nullable: true,
153+
is_primary_key: false,
154+
default: None,
155+
raw_type: None,
156+
int_width: None,
157+
float_width: None,
158+
}
159+
}
160+
161+
fn int_column(name: &str, width: Option<IntWidth>) -> ColumnInfo {
162+
ColumnInfo {
163+
name: name.to_string(),
164+
data_type: SqlDataType::Int64,
165+
nullable: true,
166+
is_primary_key: false,
167+
default: None,
168+
raw_type: None,
169+
int_width: width,
170+
float_width: None,
171+
}
172+
}
173+
174+
fn float_column(name: &str, width: Option<FloatWidth>) -> ColumnInfo {
175+
ColumnInfo {
176+
name: name.to_string(),
177+
data_type: SqlDataType::Float64,
178+
nullable: true,
179+
is_primary_key: false,
180+
default: None,
181+
raw_type: None,
182+
int_width: None,
183+
float_width: width,
184+
}
185+
}
186+
187+
fn key_value_expr(key: &str) -> Expr {
188+
Expr::Value(ValueWithSpan {
189+
value: Value::SingleQuotedString(key.to_string()),
190+
span: Span::empty(),
191+
})
192+
}
193+
194+
/// Build a single-row `INSERT ... ON CONFLICT (key) DO UPDATE SET ...`
195+
/// plan against a `key TEXT PRIMARY KEY` collection with `n INT` and
196+
/// `r REAL` columns, and return the result of range-checking `updates`.
197+
fn plan_with_on_conflict(updates: Vec<(String, SqlExpr)>) -> Result<Vec<SqlPlan>> {
198+
let declared = [
199+
string_column("key"),
200+
int_column("n", Some(IntWidth::I32)),
201+
float_column("r", Some(FloatWidth::F32)),
202+
];
203+
build_kv_insert_plan(
204+
"t".to_string(),
205+
&["key".to_string()],
206+
&[vec![key_value_expr("a")]],
207+
KvInsertIntent::Put,
208+
updates,
209+
Some("key"),
210+
&declared,
211+
)
212+
}
213+
214+
#[test]
215+
fn on_conflict_int_beyond_declared_width_is_rejected() {
216+
let updates = vec![(
217+
"n".to_string(),
218+
SqlExpr::Literal(SqlValue::Int(9_876_543_210)),
219+
)];
220+
let err = plan_with_on_conflict(updates).expect_err("i32 column must reject overflow");
221+
assert!(matches!(err, SqlError::IntegerOutOfRange { .. }));
222+
}
223+
224+
#[test]
225+
fn on_conflict_int_in_range_is_accepted() {
226+
let updates = vec![("n".to_string(), SqlExpr::Literal(SqlValue::Int(42)))];
227+
plan_with_on_conflict(updates).expect("in-range i32 literal must be accepted");
228+
}
229+
230+
#[test]
231+
fn on_conflict_float_beyond_f32_range_is_rejected() {
232+
let updates = vec![("r".to_string(), SqlExpr::Literal(SqlValue::Float(1e300)))];
233+
let err = plan_with_on_conflict(updates).expect_err("real column must reject f32 overflow");
234+
assert!(matches!(err, SqlError::FloatOutOfRange { .. }));
235+
}
236+
237+
#[test]
238+
fn on_conflict_float_rounding_is_accepted() {
239+
// Pinned per `check_declared_float_ranges`: narrowing to f32 rounds,
240+
// it does not reject, so a future change must not tighten this.
241+
let updates = vec![("r".to_string(), SqlExpr::Literal(SqlValue::Float(1.1)))];
242+
plan_with_on_conflict(updates).expect("rounding into f32 must not be rejected");
243+
}
244+
}

nodedb-sql/src/planner/dml_helpers/mod.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ pub use ast_extract::extract_point_keys;
1919
pub(super) use ast_extract::extract_table_name_from_table_with_joins;
2020
pub(super) use insert_columns::resolve_insert_columns;
2121
pub(super) use kv_insert::build_kv_insert_plan;
22-
pub(super) use range_check::{check_declared_int_ranges_in_assignments, coerce_and_check_rows};
22+
pub(super) use range_check::{
23+
check_declared_float_ranges_in_assignments, check_declared_int_ranges_in_assignments,
24+
coerce_and_check_rows,
25+
};
2326
pub(super) use value_convert::{convert_value_rows, expr_to_sql_value};
2427
pub(super) use vector_primary_insert::build_vector_primary_insert_plan;

nodedb-sql/src/planner/dml_helpers/range_check.rs

Lines changed: 155 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ pub(crate) fn coerce_and_check_rows(
3333
) -> Result<()> {
3434
coerce_rows_to_declared_types(&info.columns, rows, info.primary_key.as_deref())?;
3535
check_declared_int_ranges(&info.columns, rows)?;
36-
coerce_rows_to_declared_types(&info.columns, rows, info.primary_key.as_deref())?;
37-
check_declared_int_ranges(&info.columns, rows)
36+
check_declared_float_ranges(&info.columns, rows)
3837
}
3938

4039
/// Reject any integer value that does not fit its column's declared width.
@@ -92,6 +91,69 @@ pub(crate) fn check_declared_int_ranges(
9291
Ok(())
9392
}
9493

94+
/// Reject a finite float value that overflows to infinity when narrowed to
95+
/// its column's declared `REAL` width.
96+
///
97+
/// This is deliberately *not* the float mirror of
98+
/// [`check_declared_int_ranges`]'s full-range check: narrowing an `f64` to
99+
/// `f32` normally **rounds**, and `1.1` becoming `1.10000002` is correct
100+
/// PostgreSQL `real` behaviour — accepted, never rejected. The one narrowing
101+
/// that is not value-preserving is a finite value beyond `f32`'s range, which
102+
/// would otherwise silently become `Infinity` on read; that is refused here at
103+
/// write time, exactly as PostgreSQL refuses it (`value out of range: real`).
104+
/// A value that is *already* infinite or `NaN` in the source is legitimately
105+
/// representable in both widths and passes through unchanged — it is not an
106+
/// overflow.
107+
///
108+
/// This runs in the planner, on the same coerce-then-check path as
109+
/// [`check_declared_int_ranges`], for the same reason: the declared width is
110+
/// engine-independent, and parameters are bound into the AST before planning,
111+
/// so one check here covers literal `VALUES` and `$1` placeholders alike, for
112+
/// every engine, on every DML path.
113+
///
114+
/// See `nodedb::control::server::pgwire::numeric_narrow::checked_narrow_f32`
115+
/// for the read-side backstop this check is layered with: rows written before
116+
/// a column's width was declared, or written via non-SQL ingest, are not
117+
/// covered by this planner-time check, so the read-side guard stays in place
118+
/// as the last line of defence. Neither is redundant with the other.
119+
pub(crate) fn check_declared_float_ranges(
120+
columns: &[ColumnInfo],
121+
rows: &[Vec<(String, SqlValue)>],
122+
) -> Result<()> {
123+
// Overwhelmingly the common case — skip the per-cell name lookup entirely
124+
// when the collection declares no `REAL`-width column.
125+
if !columns
126+
.iter()
127+
.any(|c| c.float_width == Some(nodedb_types::columnar::FloatWidth::F32))
128+
{
129+
return Ok(());
130+
}
131+
132+
for row in rows {
133+
for (name, value) in row {
134+
let SqlValue::Float(v) = value else { continue };
135+
let Some(width) = columns
136+
.iter()
137+
.find(|c| c.name.eq_ignore_ascii_case(name))
138+
.and_then(|c| c.float_width)
139+
else {
140+
continue;
141+
};
142+
if width != nodedb_types::columnar::FloatWidth::F32 {
143+
continue;
144+
}
145+
if v.is_finite() && !(*v as f32).is_finite() {
146+
return Err(SqlError::FloatOutOfRange {
147+
column: name.clone(),
148+
value: *v,
149+
declared_type: width.pg_type_name(),
150+
});
151+
}
152+
}
153+
}
154+
Ok(())
155+
}
156+
95157
/// [`check_declared_int_ranges`] for `UPDATE ... SET col = <literal>`.
96158
///
97159
/// Only literal assignments are checkable at plan time; a computed assignment
@@ -116,3 +178,94 @@ pub(crate) fn check_declared_int_ranges_in_assignments(
116178
check_declared_int_ranges(columns, std::slice::from_ref(&literals))
117179
}
118180

181+
/// [`check_declared_float_ranges`] for `UPDATE ... SET col = <literal>`.
182+
///
183+
/// Same literal-only scope and same computed-assignment carve-out as
184+
/// [`check_declared_int_ranges_in_assignments`] — see its docs.
185+
pub(crate) fn check_declared_float_ranges_in_assignments(
186+
columns: &[ColumnInfo],
187+
assignments: &[(String, SqlExpr)],
188+
) -> Result<()> {
189+
let literals: Vec<(String, SqlValue)> = assignments
190+
.iter()
191+
.filter_map(|(col, expr)| match expr {
192+
SqlExpr::Literal(v @ SqlValue::Float(_)) => Some((col.clone(), v.clone())),
193+
_ => None,
194+
})
195+
.collect();
196+
if literals.is_empty() {
197+
return Ok(());
198+
}
199+
check_declared_float_ranges(columns, std::slice::from_ref(&literals))
200+
}
201+
202+
#[cfg(test)]
203+
mod float_range_tests {
204+
use super::*;
205+
use nodedb_types::columnar::FloatWidth;
206+
207+
fn float_column(name: &str, width: Option<FloatWidth>) -> ColumnInfo {
208+
ColumnInfo {
209+
name: name.to_string(),
210+
data_type: SqlDataType::Float64,
211+
nullable: true,
212+
is_primary_key: false,
213+
default: None,
214+
raw_type: None,
215+
int_width: None,
216+
float_width: width,
217+
}
218+
}
219+
220+
fn row(name: &str, value: f64) -> Vec<Vec<(String, SqlValue)>> {
221+
vec![vec![(name.to_string(), SqlValue::Float(value))]]
222+
}
223+
224+
#[test]
225+
fn out_of_f32_range_literal_into_real_column_is_rejected() {
226+
let columns = [float_column("r", Some(FloatWidth::F32))];
227+
let rows = row("r", 1e300);
228+
let err = check_declared_float_ranges(&columns, &rows).expect_err("must overflow f32");
229+
assert!(matches!(err, SqlError::FloatOutOfRange { .. }));
230+
}
231+
232+
#[test]
233+
fn same_literal_into_double_column_is_accepted() {
234+
let columns = [float_column("r", Some(FloatWidth::F64))];
235+
let rows = row("r", 1e300);
236+
check_declared_float_ranges(&columns, &rows).expect("f64 has no narrower width to check");
237+
}
238+
239+
#[test]
240+
fn merely_rounding_value_into_real_column_is_accepted() {
241+
// The load-bearing case: `1.1` narrows to `1.10000002` under `f32`,
242+
// which is correct rounding, never an error.
243+
let columns = [float_column("r", Some(FloatWidth::F32))];
244+
let rows = row("r", 1.1);
245+
check_declared_float_ranges(&columns, &rows).expect("rounding must not be rejected");
246+
}
247+
248+
#[test]
249+
fn f32_max_itself_is_accepted() {
250+
let columns = [float_column("r", Some(FloatWidth::F32))];
251+
let rows = row("r", f32::MAX as f64);
252+
check_declared_float_ranges(&columns, &rows).expect("f32::MAX fits f32 exactly");
253+
}
254+
255+
#[test]
256+
fn already_infinite_or_nan_literal_is_accepted() {
257+
let columns = [float_column("r", Some(FloatWidth::F32))];
258+
for v in [f64::INFINITY, f64::NEG_INFINITY, f64::NAN] {
259+
let rows = row("r", v);
260+
check_declared_float_ranges(&columns, &rows)
261+
.expect("already-infinite/NaN source values are not an overflow");
262+
}
263+
}
264+
265+
#[test]
266+
fn no_declared_float_width_skips_check() {
267+
let columns = [float_column("r", None)];
268+
let rows = row("r", 1e300);
269+
check_declared_float_ranges(&columns, &rows).expect("untyped float column is unchecked");
270+
}
271+
}

nodedb-sql/src/planner/dml_update_delete.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ use super::super::ast_helpers::{
99
flatten_and_expr, qualified_ident_pair, strip_and_convert_filters,
1010
};
1111
use super::super::dml_helpers::{
12-
check_declared_int_ranges_in_assignments, extract_point_keys,
13-
extract_table_name_from_table_with_joins,
12+
check_declared_float_ranges_in_assignments, check_declared_int_ranges_in_assignments,
13+
extract_point_keys, extract_table_name_from_table_with_joins,
1414
};
1515
use crate::engine_rules::{self, DeleteParams, UpdateFromParams, UpdateParams};
1616
use crate::error::{Result, SqlError};
@@ -54,6 +54,7 @@ pub fn plan_update(stmt: &ast::Statement, catalog: &dyn SqlCatalog) -> Result<Ve
5454
// an `INSERT` that does (see `declared_type_coerce`).
5555
coerce_assignments_to_declared_types(&info.columns, &mut assigns, info.primary_key.as_deref())?;
5656
check_declared_int_ranges_in_assignments(&info.columns, &assigns)?;
57+
check_declared_float_ranges_in_assignments(&info.columns, &assigns)?;
5758

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

0 commit comments

Comments
 (0)