From ee55116ca676ce2db39bb396e36c270fee1e4532 Mon Sep 17 00:00:00 2001 From: Pere Diaz Bou Date: Thu, 27 Mar 2025 15:53:03 +0100 Subject: [PATCH] return row as reference to registers --- bindings/go/rs_src/rows.rs | 6 +- bindings/go/rs_src/types.rs | 19 +++++ bindings/java/rs_src/limbo_statement.rs | 12 +-- bindings/javascript/src/lib.rs | 16 ++-- bindings/python/src/lib.rs | 12 +-- bindings/rust/src/lib.rs | 2 +- bindings/wasm/lib.rs | 12 +-- cli/app.rs | 20 ++--- core/lib.rs | 2 +- core/vdbe/mod.rs | 80 ++++++++++++++++++- simulator/generation/plan.rs | 10 +-- sqlite3/src/lib.rs | 5 +- tests/integration/fuzz/mod.rs | 13 ++- .../query_processing/test_write_path.rs | 22 +++-- tests/integration/wal/test_wal.rs | 2 +- 15 files changed, 166 insertions(+), 67 deletions(-) diff --git a/bindings/go/rs_src/rows.rs b/bindings/go/rs_src/rows.rs index a1e6382d9..2de0bf8a2 100644 --- a/bindings/go/rs_src/rows.rs +++ b/bindings/go/rs_src/rows.rs @@ -2,7 +2,7 @@ use crate::{ types::{LimboValue, ResultCode}, LimboConn, }; -use limbo_core::{LimboError, Statement, StepResult}; +use limbo_core::{LimboError, OwnedValue, Statement, StepResult}; use std::ffi::{c_char, c_void}; pub struct LimboRows<'conn> { @@ -75,8 +75,8 @@ pub extern "C" fn rows_get_value(ctx: *mut c_void, col_idx: usize) -> *const c_v let ctx = LimboRows::from_ptr(ctx); if let Some(row) = ctx.stmt.row() { - if let Some(value) = row.get_values().get(col_idx) { - return LimboValue::from_value(value).to_ptr(); + if let Ok(value) = row.get::<&OwnedValue>(col_idx) { + return LimboValue::from_owned_value(value).to_ptr(); } } std::ptr::null() diff --git a/bindings/go/rs_src/types.rs b/bindings/go/rs_src/types.rs index 7bded37ae..712306a34 100644 --- a/bindings/go/rs_src/types.rs +++ b/bindings/go/rs_src/types.rs @@ -142,6 +142,25 @@ impl LimboValue { Box::into_raw(Box::new(self)) as *const c_void } + pub fn from_owned_value(value: &limbo_core::OwnedValue) -> Self { + match value { + limbo_core::OwnedValue::Integer(i) => { + LimboValue::new(ValueType::Integer, ValueUnion::from_int(*i)) + } + limbo_core::OwnedValue::Float(r) => { + LimboValue::new(ValueType::Real, ValueUnion::from_real(*r)) + } + limbo_core::OwnedValue::Text(s) => { + LimboValue::new(ValueType::Text, ValueUnion::from_str(s.as_str())) + } + limbo_core::OwnedValue::Blob(b) => { + LimboValue::new(ValueType::Blob, ValueUnion::from_bytes(b.as_slice())) + } + limbo_core::OwnedValue::Null => { + LimboValue::new(ValueType::Null, ValueUnion::from_null()) + } + } + } pub fn from_value(value: &limbo_core::RefValue) -> Self { match value { limbo_core::RefValue::Integer(i) => { diff --git a/bindings/java/rs_src/limbo_statement.rs b/bindings/java/rs_src/limbo_statement.rs index fbb33fa79..b28ff55b1 100644 --- a/bindings/java/rs_src/limbo_statement.rs +++ b/bindings/java/rs_src/limbo_statement.rs @@ -104,17 +104,17 @@ fn row_to_obj_array<'local>( ) -> Result> { let obj_array = env.new_object_array(row.len() as i32, "java/lang/Object", JObject::null())?; - for (i, value) in row.get_values().iter().enumerate() { + for (i, value) in row.get_values().enumerate() { let obj = match value { - limbo_core::RefValue::Null => JObject::null(), - limbo_core::RefValue::Integer(i) => { + limbo_core::OwnedValue::Null => JObject::null(), + limbo_core::OwnedValue::Integer(i) => { env.new_object("java/lang/Long", "(J)V", &[JValue::Long(*i)])? } - limbo_core::RefValue::Float(f) => { + limbo_core::OwnedValue::Float(f) => { env.new_object("java/lang/Double", "(D)V", &[JValue::Double(*f)])? } - limbo_core::RefValue::Text(s) => env.new_string(s.as_str())?.into(), - limbo_core::RefValue::Blob(b) => env.byte_array_from_slice(&b.to_slice())?.into(), + limbo_core::OwnedValue::Text(s) => env.new_string(s.as_str())?.into(), + limbo_core::OwnedValue::Blob(b) => env.byte_array_from_slice(&b.as_slice())?.into(), }; if let Err(e) = env.set_object_array_element(&obj_array, i as i32, obj) { eprintln!("Error on parsing row: {:?}", e); diff --git a/bindings/javascript/src/lib.rs b/bindings/javascript/src/lib.rs index 2db1fec48..614b17677 100644 --- a/bindings/javascript/src/lib.rs +++ b/bindings/javascript/src/lib.rs @@ -76,7 +76,7 @@ impl Statement { Ok(limbo_core::StepResult::Row) => { let row = stmt.row().unwrap(); let mut obj = env.create_object()?; - for (idx, value) in row.get_values().iter().enumerate() { + for (idx, value) in row.get_values().enumerate() { let key = stmt.get_column_name(idx); let js_value = to_js_value(&env, value); obj.set_named_property(&key, js_value)?; @@ -92,14 +92,14 @@ impl Statement { } } -fn to_js_value(env: &napi::Env, value: &limbo_core::RefValue) -> JsUnknown { +fn to_js_value(env: &napi::Env, value: &limbo_core::OwnedValue) -> JsUnknown { match value { - limbo_core::RefValue::Null => env.get_null().unwrap().into_unknown(), - limbo_core::RefValue::Integer(i) => env.create_int64(*i).unwrap().into_unknown(), - limbo_core::RefValue::Float(f) => env.create_double(*f).unwrap().into_unknown(), - limbo_core::RefValue::Text(s) => env.create_string(s.as_str()).unwrap().into_unknown(), - limbo_core::RefValue::Blob(b) => { - env.create_buffer_copy(b.to_slice()).unwrap().into_unknown() + limbo_core::OwnedValue::Null => env.get_null().unwrap().into_unknown(), + limbo_core::OwnedValue::Integer(i) => env.create_int64(*i).unwrap().into_unknown(), + limbo_core::OwnedValue::Float(f) => env.create_double(*f).unwrap().into_unknown(), + limbo_core::OwnedValue::Text(s) => env.create_string(s.as_str()).unwrap().into_unknown(), + limbo_core::OwnedValue::Blob(b) => { + env.create_buffer_copy(b.as_slice()).unwrap().into_unknown() } } } diff --git a/bindings/python/src/lib.rs b/bindings/python/src/lib.rs index ff963f1db..851136b50 100644 --- a/bindings/python/src/lib.rs +++ b/bindings/python/src/lib.rs @@ -326,11 +326,13 @@ fn row_to_py(py: Python, row: &limbo_core::Row) -> Result { let mut py_values = Vec::new(); for value in row.get_values() { match value { - limbo_core::RefValue::Null => py_values.push(py.None()), - limbo_core::RefValue::Integer(i) => py_values.push(i.into_pyobject(py)?.into()), - limbo_core::RefValue::Float(f) => py_values.push(f.into_pyobject(py)?.into()), - limbo_core::RefValue::Text(s) => py_values.push(s.as_str().into_pyobject(py)?.into()), - limbo_core::RefValue::Blob(b) => py_values.push(PyBytes::new(py, b.to_slice()).into()), + limbo_core::OwnedValue::Null => py_values.push(py.None()), + limbo_core::OwnedValue::Integer(i) => py_values.push(i.into_pyobject(py)?.into()), + limbo_core::OwnedValue::Float(f) => py_values.push(f.into_pyobject(py)?.into()), + limbo_core::OwnedValue::Text(s) => py_values.push(s.as_str().into_pyobject(py)?.into()), + limbo_core::OwnedValue::Blob(b) => { + py_values.push(PyBytes::new(py, b.as_slice()).into()) + } } } Ok(PyTuple::new(py, &py_values) diff --git a/bindings/rust/src/lib.rs b/bindings/rust/src/lib.rs index 781a40c94..fe653fdb5 100644 --- a/bindings/rust/src/lib.rs +++ b/bindings/rust/src/lib.rs @@ -212,7 +212,7 @@ impl Rows { Ok(limbo_core::StepResult::Row) => { let row = stmt.row().unwrap(); Ok(Some(Row { - values: row.get_values().iter().map(|v| v.to_owned()).collect(), + values: row.get_values().map(|v| v.to_owned()).collect(), })) } _ => Ok(None), diff --git a/bindings/wasm/lib.rs b/bindings/wasm/lib.rs index 330d4dd68..3a5819efc 100644 --- a/bindings/wasm/lib.rs +++ b/bindings/wasm/lib.rs @@ -177,10 +177,10 @@ impl Statement { } } -fn to_js_value(value: &limbo_core::RefValue) -> JsValue { +fn to_js_value(value: &limbo_core::OwnedValue) -> JsValue { match value { - limbo_core::RefValue::Null => JsValue::null(), - limbo_core::RefValue::Integer(i) => { + limbo_core::OwnedValue::Null => JsValue::null(), + limbo_core::OwnedValue::Integer(i) => { let i = *i; if i >= i32::MIN as i64 && i <= i32::MAX as i64 { JsValue::from(i as i32) @@ -188,9 +188,9 @@ fn to_js_value(value: &limbo_core::RefValue) -> JsValue { JsValue::from(i) } } - limbo_core::RefValue::Float(f) => JsValue::from(*f), - limbo_core::RefValue::Text(t) => JsValue::from_str(t.as_str()), - limbo_core::RefValue::Blob(b) => js_sys::Uint8Array::from(b.to_slice()).into(), + limbo_core::OwnedValue::Float(f) => JsValue::from(*f), + limbo_core::OwnedValue::Text(t) => JsValue::from_str(t.as_str()), + limbo_core::OwnedValue::Blob(b) => js_sys::Uint8Array::from(b.as_slice()).into(), } } diff --git a/cli/app.rs b/cli/app.rs index c85f20d61..f6d53ee19 100644 --- a/cli/app.rs +++ b/cli/app.rs @@ -5,7 +5,7 @@ use crate::{ opcodes_dictionary::OPCODE_DESCRIPTIONS, }; use comfy_table::{Attribute, Cell, CellAlignment, Color, ContentArrangement, Row, Table}; -use limbo_core::{Database, LimboError, RefValue, Statement, StepResult}; +use limbo_core::{Database, LimboError, OwnedValue, RefValue, Statement, StepResult}; use clap::{Parser, ValueEnum}; use rustyline::{history::DefaultHistory, Editor}; @@ -711,7 +711,7 @@ impl<'a> Limbo<'a> { match rows.step() { Ok(StepResult::Row) => { let row = rows.row().unwrap(); - for (i, value) in row.get_values().iter().enumerate() { + for (i, value) in row.get_values().enumerate() { if i > 0 { let _ = self.writer.write(b"|"); } @@ -767,21 +767,21 @@ impl<'a> Limbo<'a> { let record = rows.row().unwrap(); let mut row = Row::new(); row.max_height(1); - for (idx, value) in record.get_values().iter().enumerate() { + for (idx, value) in record.get_values().enumerate() { let (content, alignment) = match value { - RefValue::Null => { + OwnedValue::Null => { (self.opts.null_value.clone(), CellAlignment::Left) } - RefValue::Integer(_) => { + OwnedValue::Integer(_) => { (format!("{}", value), CellAlignment::Right) } - RefValue::Float(_) => { + OwnedValue::Float(_) => { (format!("{}", value), CellAlignment::Right) } - RefValue::Text(_) => { + OwnedValue::Text(_) => { (format!("{}", value), CellAlignment::Left) } - RefValue::Blob(_) => { + OwnedValue::Blob(_) => { (format!("{}", value), CellAlignment::Left) } }; @@ -849,7 +849,7 @@ impl<'a> Limbo<'a> { match rows.step()? { StepResult::Row => { let row = rows.row().unwrap(); - if let Some(RefValue::Text(schema)) = row.get_values().first() { + if let Ok(OwnedValue::Text(schema)) = row.get::<&OwnedValue>(0) { let _ = self.write_fmt(format_args!("{};", schema.as_str())); found = true; } @@ -907,7 +907,7 @@ impl<'a> Limbo<'a> { match rows.step()? { StepResult::Row => { let row = rows.row().unwrap(); - if let Some(RefValue::Text(table)) = row.get_values().first() { + if let Ok(OwnedValue::Text(table)) = row.get::<&OwnedValue>(0) { tables.push_str(table.as_str()); tables.push(' '); } diff --git a/core/lib.rs b/core/lib.rs index 0314d9b81..f1a1b37a0 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -597,7 +597,7 @@ impl Statement { } } -pub type Row = types::ImmutableRecord; +pub type Row = vdbe::Row; pub type StepResult = vdbe::StepResult; diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 53d3095c1..f530a33fa 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -239,12 +239,19 @@ pub enum Register { Record(ImmutableRecord), } +/// A row is a the list of registers that hold the values for a filtered row. This row is a pointer, therefore +/// after stepping again, row will be invalidated to be sure it doesn't point to somewhere unexpected. +pub struct Row { + values: *const Register, + count: usize, +} + /// The program state describes the environment in which the program executes. pub struct ProgramState { pub pc: InsnReference, cursors: RefCell>>, registers: Vec, - pub(crate) result_row: Option, + pub(crate) result_row: Option, last_compare: Option, deferred_seek: Option<(CursorID, CursorID)>, ended_coroutine: Bitfield<4>, // flag to indicate that a coroutine has ended (key is the yield register. currently we assume that the yield register is always between 0-255, YOLO) @@ -403,6 +410,8 @@ impl Program { if state.is_interrupted() { return Ok(StepResult::Interrupt); } + // invalidate row + let _ = state.result_row.take(); let insn = &self.insns[state.pc as usize]; trace_insn(self, state.pc as InsnReference, insn); match insn { @@ -1257,8 +1266,12 @@ impl Program { state.pc += 1; } Insn::ResultRow { start_reg, count } => { - let record = make_record(&state.registers, start_reg, count); - state.result_row = Some(record); + let row = Row { + values: &state.registers[*start_reg] as *const Register, + count: *count, + }; + + state.result_row = Some(row); state.pc += 1; return Ok(StepResult::Row); } @@ -4486,6 +4499,67 @@ fn exec_math_log(arg: &OwnedValue, base: Option<&OwnedValue>) -> OwnedValue { OwnedValue::Float(result) } +pub trait FromValueRow<'a> { + fn from_value(value: &'a OwnedValue) -> Result + where + Self: Sized + 'a; +} + +impl<'a> FromValueRow<'a> for i64 { + fn from_value(value: &'a OwnedValue) -> Result { + match value { + OwnedValue::Integer(i) => Ok(*i), + _ => Err(LimboError::ConversionError("Expected integer value".into())), + } + } +} + +impl<'a> FromValueRow<'a> for String { + fn from_value(value: &'a OwnedValue) -> Result { + match value { + OwnedValue::Text(s) => Ok(s.as_str().to_string()), + _ => Err(LimboError::ConversionError("Expected text value".into())), + } + } +} + +impl<'a> FromValueRow<'a> for &'a str { + fn from_value(value: &'a OwnedValue) -> Result { + match value { + OwnedValue::Text(s) => Ok(s.as_str()), + _ => Err(LimboError::ConversionError("Expected text value".into())), + } + } +} + +impl<'a> FromValueRow<'a> for &'a OwnedValue { + fn from_value(value: &'a OwnedValue) -> Result { + Ok(value) + } +} + +impl Row { + pub fn get<'a, T: FromValueRow<'a> + 'a>(&'a self, idx: usize) -> Result { + let value = unsafe { self.values.add(idx).as_ref().unwrap() }; + let value = match value { + Register::OwnedValue(owned_value) => owned_value, + _ => unreachable!("a row should be formed of values only"), + }; + T::from_value(value) + } + + pub fn get_values(&self) -> impl Iterator { + let values = unsafe { std::slice::from_raw_parts(self.values, self.count) }; + dbg!(&values); + // This should be ownedvalues + // TODO: add check for this + values.iter().map(|v| v.get_owned_value()) + } + + pub fn len(&self) -> usize { + self.count + } +} #[cfg(test)] mod tests { use crate::vdbe::{exec_replace, Register}; diff --git a/simulator/generation/plan.rs b/simulator/generation/plan.rs index 2799976a2..ecad92344 100644 --- a/simulator/generation/plan.rs +++ b/simulator/generation/plan.rs @@ -494,13 +494,13 @@ impl Interaction { let mut r = Vec::new(); for v in row.get_values() { let v = match v { - limbo_core::RefValue::Null => Value::Null, - limbo_core::RefValue::Integer(i) => Value::Integer(*i), - limbo_core::RefValue::Float(f) => Value::Float(*f), - limbo_core::RefValue::Text(t) => { + limbo_core::OwnedValue::Null => Value::Null, + limbo_core::OwnedValue::Integer(i) => Value::Integer(*i), + limbo_core::OwnedValue::Float(f) => Value::Float(*f), + limbo_core::OwnedValue::Text(t) => { Value::Text(t.as_str().to_string()) } - limbo_core::RefValue::Blob(b) => Value::Blob(b.to_slice().to_vec()), + limbo_core::OwnedValue::Blob(b) => Value::Blob(b.to_vec()), }; r.push(v); } diff --git a/sqlite3/src/lib.rs b/sqlite3/src/lib.rs index 85799c364..f54e6bdb2 100644 --- a/sqlite3/src/lib.rs +++ b/sqlite3/src/lib.rs @@ -1,6 +1,7 @@ #![allow(clippy::missing_safety_doc)] #![allow(non_camel_case_types)] +use limbo_core::OwnedValue; use log::trace; use std::ffi::{self, CStr, CString}; @@ -636,8 +637,8 @@ pub unsafe extern "C" fn sqlite3_column_text( Some(row) => row, None => return std::ptr::null(), }; - match row.get_values().get(idx as usize) { - Some(limbo_core::RefValue::Text(text)) => text.as_str().as_ptr(), + match row.get::<&OwnedValue>(idx as usize) { + Ok(limbo_core::OwnedValue::Text(text)) => text.as_str().as_ptr(), _ => std::ptr::null(), } } diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index 881ab4e54..f776fc9a7 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -72,17 +72,14 @@ mod tests { }; let row = row .get_values() - .iter() .map(|x| match x { - limbo_core::RefValue::Null => rusqlite::types::Value::Null, - limbo_core::RefValue::Integer(x) => rusqlite::types::Value::Integer(*x), - limbo_core::RefValue::Float(x) => rusqlite::types::Value::Real(*x), - limbo_core::RefValue::Text(x) => { + limbo_core::OwnedValue::Null => rusqlite::types::Value::Null, + limbo_core::OwnedValue::Integer(x) => rusqlite::types::Value::Integer(*x), + limbo_core::OwnedValue::Float(x) => rusqlite::types::Value::Real(*x), + limbo_core::OwnedValue::Text(x) => { rusqlite::types::Value::Text(x.as_str().to_string()) } - limbo_core::RefValue::Blob(x) => { - rusqlite::types::Value::Blob(x.to_slice().to_vec()) - } + limbo_core::OwnedValue::Blob(x) => rusqlite::types::Value::Blob(x.to_vec()), }) .collect(); rows.push(row); diff --git a/tests/integration/query_processing/test_write_path.rs b/tests/integration/query_processing/test_write_path.rs index 0dd438f2e..543e0678a 100644 --- a/tests/integration/query_processing/test_write_path.rs +++ b/tests/integration/query_processing/test_write_path.rs @@ -1,6 +1,6 @@ use crate::common::{self, maybe_setup_tracing}; use crate::common::{compare_string, do_flush, TempDatabase}; -use limbo_core::{Connection, RefValue, StepResult}; +use limbo_core::{Connection, OwnedValue, RefValue, StepResult}; use log::debug; use std::rc::Rc; @@ -192,10 +192,10 @@ fn test_sequential_write() -> anyhow::Result<()> { match rows.step()? { StepResult::Row => { let row = rows.row().unwrap(); - let first_value = row.get_values().first().expect("missing id"); + let first_value = row.get::<&OwnedValue>(0).expect("missing id"); let id = match first_value { - limbo_core::RefValue::Integer(i) => *i as i32, - limbo_core::RefValue::Float(f) => *f as i32, + limbo_core::OwnedValue::Integer(i) => *i as i32, + limbo_core::OwnedValue::Float(f) => *f as i32, _ => unreachable!(), }; assert_eq!(current_read_index, id); @@ -258,9 +258,9 @@ fn test_regression_multi_row_insert() -> anyhow::Result<()> { match rows.step()? { StepResult::Row => { let row = rows.row().unwrap(); - let first_value = row.get_values().first().expect("missing id"); + let first_value = row.get::<&OwnedValue>(0).expect("missing id"); let id = match first_value { - RefValue::Float(f) => *f as i32, + OwnedValue::Float(f) => *f as i32, _ => panic!("expected float"), }; actual_ids.push(id); @@ -304,7 +304,10 @@ fn test_statement_reset() -> anyhow::Result<()> { match stmt.step()? { StepResult::Row => { let row = stmt.row().unwrap(); - assert_eq!(*row.get_value(0), limbo_core::OwnedValue::Integer(1)); + assert_eq!( + *row.get::<&OwnedValue>(0), + limbo_core::OwnedValue::Integer(1) + ); break; } StepResult::IO => tmp_db.io.run_once()?, @@ -318,7 +321,10 @@ fn test_statement_reset() -> anyhow::Result<()> { match stmt.step()? { StepResult::Row => { let row = stmt.row().unwrap(); - assert_eq!(*row.get_value(0), limbo_core::OwnedValue::Integer(1)); + assert_eq!( + *row.get::<&OwnedValue>(0), + limbo_core::OwnedValue::Integer(1) + ); break; } StepResult::IO => tmp_db.io.run_once()?, diff --git a/tests/integration/wal/test_wal.rs b/tests/integration/wal/test_wal.rs index 4cf829919..53790a7df 100644 --- a/tests/integration/wal/test_wal.rs +++ b/tests/integration/wal/test_wal.rs @@ -158,7 +158,7 @@ pub(crate) fn execute_and_get_ints( let row = stmt.row().unwrap(); for value in row.get_values() { let out = match value { - limbo_core::RefValue::Integer(i) => i, + limbo_core::OwnedValue::Integer(i) => i, _ => { return Err(LimboError::ConversionError(format!( "cannot convert {value} to int"