add review comments

This commit is contained in:
bit-aloo
2025-08-24 15:14:39 +05:30
parent 9bebc9b5c7
commit a3b87cd97f
10 changed files with 54 additions and 165 deletions

View File

@@ -1,7 +1,8 @@
use crate::schema::{Index, IndexColumn, Schema};
use crate::translate::emitter::{emit_query, LimitCtx, TranslateCtx};
use crate::translate::expr::translate_expr;
use crate::translate::plan::{Plan, QueryDestination, SelectPlan};
use crate::translate::result_row::{build_limit_offset_expr, try_fold_expr_to_i64};
use crate::translate::result_row::try_fold_expr_to_i64;
use crate::vdbe::builder::{CursorType, ProgramBuilder};
use crate::vdbe::insn::Insn;
use crate::vdbe::BranchOffset;
@@ -38,11 +39,18 @@ pub fn emit_program_for_compound_select(
return Ok(());
}
let right_most_ctx = TranslateCtx::new(
program,
schema,
syms,
right_most.table_references.joined_tables().len(),
);
// Each subselect shares the same limit_ctx and offset, because the LIMIT, OFFSET applies to
// the entire compound select, not just a single subselect.
let limit_ctx = limit.clone().map(|limit| {
let limit_ctx = limit.as_ref().map(|limit| {
let reg = program.alloc_register();
if let Some(val) = try_fold_expr_to_i64(&limit) {
if let Some(val) = try_fold_expr_to_i64(limit) {
program.emit_insn(Insn::Integer {
value: val,
dest: reg,
@@ -52,7 +60,7 @@ pub fn emit_program_for_compound_select(
let label_zero = program.allocate_label();
build_limit_offset_expr(program, reg, &limit);
_ = translate_expr(program, None, limit, reg, &right_most_ctx.resolver);
program.emit_insn(Insn::MustBeInt { reg });
@@ -87,7 +95,7 @@ pub fn emit_program_for_compound_select(
let label_zero = program.allocate_label();
build_limit_offset_expr(program, reg, offset_expr);
_ = translate_expr(program, None, offset_expr, reg, &right_most_ctx.resolver);
program.emit_insn(Insn::MustBeInt { reg });

View File

@@ -2,7 +2,7 @@ use crate::schema::Table;
use crate::translate::emitter::emit_program;
use crate::translate::optimizer::optimize_plan;
use crate::translate::plan::{DeletePlan, Operation, Plan};
use crate::translate::planner::{parse_limit, parse_where};
use crate::translate::planner::parse_where;
use crate::util::normalize_ident;
use crate::vdbe::builder::{ProgramBuilder, ProgramBuilderOpts, TableRefIdCounter};
use crate::{schema::Schema, Result, SymbolTable};
@@ -107,7 +107,8 @@ pub fn prepare_delete_plan(
)?;
// Parse the LIMIT/OFFSET clause
let (resolved_limit, resolved_offset) = limit.map_or(Ok((None, None)), |l| parse_limit(&l))?;
let (resolved_limit, resolved_offset) =
limit.map_or((None, None), |l| (Some(l.expr), l.offset));
let plan = DeletePlan {
table_references,

View File

@@ -217,7 +217,7 @@ impl fmt::Display for UpdatePlan {
)?;
}
}
if let Some(limit) = self.limit.clone() {
if let Some(limit) = self.limit.as_ref() {
writeln!(f, "LIMIT: {limit}")?;
}
if let Some(ret) = &self.returning {

View File

@@ -26,7 +26,7 @@ use crate::schema::{BTreeTable, Column, Schema, Table};
use crate::translate::compound_select::emit_program_for_compound_select;
use crate::translate::expr::{emit_returning_results, ReturningValueRegisters};
use crate::translate::plan::{DeletePlan, Plan, QueryDestination, Search};
use crate::translate::result_row::{build_limit_offset_expr, try_fold_expr_to_i64};
use crate::translate::result_row::try_fold_expr_to_i64;
use crate::translate::values::emit_values;
use crate::util::exprs_are_equivalent;
use crate::vdbe::builder::{CursorKey, CursorType, ProgramBuilder};
@@ -257,7 +257,7 @@ pub fn emit_query<'a>(
// Emit subqueries first so the results can be read in the main query loop.
emit_subqueries(program, t_ctx, &mut plan.table_references)?;
init_limit(program, t_ctx, plan.limit.clone(), plan.offset.clone());
init_limit(program, t_ctx, &plan.limit, &plan.offset);
// No rows will be read from source table loops if there is a constant false condition eg. WHERE 0
// however an aggregation might still happen,
@@ -413,7 +413,7 @@ fn emit_program_for_delete(
}
}
init_limit(program, &mut t_ctx, plan.limit, None);
init_limit(program, &mut t_ctx, &plan.limit, &None);
// No rows will be read from source table loops if there is a constant false condition eg. WHERE 0
let after_main_loop_label = program.allocate_label();
@@ -671,7 +671,7 @@ fn emit_program_for_update(
}
}
init_limit(program, &mut t_ctx, plan.limit.clone(), plan.offset.clone());
init_limit(program, &mut t_ctx, &plan.limit, &plan.offset);
let after_main_loop_label = program.allocate_label();
t_ctx.label_main_loop_end = Some(after_main_loop_label);
if plan.contains_constant_false_condition {
@@ -1552,8 +1552,8 @@ pub fn emit_cdc_insns(
fn init_limit(
program: &mut ProgramBuilder,
t_ctx: &mut TranslateCtx,
limit: Option<Box<Expr>>,
offset: Option<Box<Expr>>,
limit: &Option<Box<Expr>>,
offset: &Option<Box<Expr>>,
) {
if t_ctx.limit_ctx.is_none() && limit.is_some() {
t_ctx.limit_ctx = Some(LimitCtx::new(program));
@@ -1563,7 +1563,7 @@ fn init_limit(
};
if limit_ctx.initialize_counter {
if let Some(expr) = limit {
if let Some(value) = try_fold_expr_to_i64(&expr) {
if let Some(value) = try_fold_expr_to_i64(expr) {
program.emit_insn(Insn::Integer {
value,
dest: limit_ctx.reg_limit,
@@ -1572,7 +1572,8 @@ fn init_limit(
let r = limit_ctx.reg_limit;
program.add_comment(program.offset(), "OFFSET expr");
let label_zero = program.allocate_label();
build_limit_offset_expr(program, r, &expr);
_ = translate_expr(program, None, expr, r, &t_ctx.resolver);
program.emit_insn(Insn::MustBeInt { reg: r });
program.emit_insn(Insn::IfNeg {
reg: r,
@@ -1590,7 +1591,7 @@ fn init_limit(
if t_ctx.reg_offset.is_none() {
if let Some(expr) = offset {
if let Some(value) = try_fold_expr_to_i64(&expr) {
if let Some(value) = try_fold_expr_to_i64(expr) {
if value != 0 {
let reg = program.alloc_register();
t_ctx.reg_offset = Some(reg);
@@ -1609,7 +1610,8 @@ fn init_limit(
let r = reg;
program.add_comment(program.offset(), "OFFSET expr");
let label_zero = program.allocate_label();
build_limit_offset_expr(program, r, &expr);
_ = translate_expr(program, None, expr, r, &t_ctx.resolver);
program.emit_insn(Insn::MustBeInt { reg: r });
program.emit_insn(Insn::IfNeg {
reg: r,

View File

@@ -117,7 +117,13 @@ pub fn emit_order_by(
});
program.preassign_label_to_next_insn(sort_loop_start_label);
emit_offset(program, plan, sort_loop_next_label, t_ctx.reg_offset);
emit_offset(
program,
plan,
sort_loop_next_label,
t_ctx.reg_offset,
&t_ctx.resolver,
);
program.emit_insn(Insn::SorterData {
cursor_id: sort_cursor,

View File

@@ -21,8 +21,8 @@ use crate::{
};
use turso_parser::ast::Literal::Null;
use turso_parser::ast::{
self, As, Expr, FromClause, JoinType, Limit, Literal, Materialized, QualifiedName,
TableInternalId, With,
self, As, Expr, FromClause, JoinType, Literal, Materialized, QualifiedName, TableInternalId,
With,
};
pub const ROWID: &str = "rowid";
@@ -1106,15 +1106,6 @@ fn parse_join(
Ok(())
}
#[allow(clippy::type_complexity)]
pub fn parse_limit(limit: &Limit) -> Result<(Option<Box<Expr>>, Option<Box<Expr>>)> {
let limit_expr = Some(limit.expr.clone());
let offset_expr = limit.offset.clone();
Ok((limit_expr, offset_expr))
}
pub fn break_predicate_at_and_boundaries(predicate: &Expr, out_predicates: &mut Vec<Expr>) {
match predicate {
Expr::Binary(left, ast::Operator::And, right) => {

View File

@@ -1,10 +1,9 @@
use turso_parser::ast::{Expr, Literal, Name, Operator, UnaryOperator};
use crate::{
error::SQLITE_CONSTRAINT,
vdbe::{
builder::ProgramBuilder,
insn::{CmpInsFlags, IdxInsertFlags, InsertFlags, Insn},
insn::{IdxInsertFlags, InsertFlags, Insn},
BranchOffset,
},
Result,
@@ -33,7 +32,7 @@ pub fn emit_select_result(
limit_ctx: Option<LimitCtx>,
) -> Result<()> {
if let (Some(jump_to), Some(_)) = (offset_jump_to, label_on_limit_reached) {
emit_offset(program, plan, jump_to, reg_offset);
emit_offset(program, plan, jump_to, reg_offset, resolver);
}
let start_reg = reg_result_cols_start;
@@ -166,12 +165,13 @@ pub fn emit_offset(
plan: &SelectPlan,
jump_to: BranchOffset,
reg_offset: Option<usize>,
resolver: &Resolver,
) {
let Some(offset_expr) = &plan.offset else {
return;
};
if let Some(val) = try_fold_expr_to_i64(&offset_expr.clone()) {
if let Some(val) = try_fold_expr_to_i64(offset_expr) {
if val > 0 {
program.add_comment(program.offset(), "OFFSET const");
program.emit_insn(Insn::IfPos {
@@ -189,7 +189,7 @@ pub fn emit_offset(
let label_zero = program.allocate_label();
build_limit_offset_expr(program, r, offset_expr);
_ = translate_expr(program, None, offset_expr, r, resolver);
program.emit_insn(Insn::MustBeInt { reg: r });
@@ -212,126 +212,6 @@ pub fn emit_offset(
program.emit_insn(Insn::Integer { value: 0, dest: r });
}
pub fn build_limit_offset_expr(program: &mut ProgramBuilder, r: usize, expr: &Expr) {
match expr {
Expr::Literal(Literal::Numeric(n)) => {
let value = n.parse::<i64>().unwrap_or_else(|_| {
program.emit_insn(Insn::Halt {
err_code: SQLITE_CONSTRAINT,
description: "invalid numeric literal".into(),
});
0
});
program.emit_int(value, r);
}
Expr::Literal(Literal::Null) => {
program.emit_int(0, r);
}
Expr::Id(Name::Ident(s)) => {
let lowered = s.to_ascii_lowercase();
if lowered == "true" {
program.emit_int(1, r);
} else if lowered == "false" {
program.emit_int(0, r);
} else {
program.emit_insn(Insn::Halt {
err_code: SQLITE_CONSTRAINT,
description: format!("invalid boolean string literal: {s}"),
});
}
}
Expr::Unary(UnaryOperator::Negative, inner) => {
let inner_reg = program.alloc_register();
build_limit_offset_expr(program, inner_reg, inner);
let neg_one_reg = program.alloc_register();
program.emit_int(-1, neg_one_reg);
program.emit_insn(Insn::Multiply {
lhs: inner_reg,
rhs: neg_one_reg,
dest: r,
});
}
Expr::Unary(UnaryOperator::Positive, inner) => {
let inner_reg = program.alloc_register();
build_limit_offset_expr(program, inner_reg, inner);
program.emit_insn(Insn::Copy {
src_reg: inner_reg,
dst_reg: r,
extra_amount: 0,
});
}
Expr::Binary(left, op, right) => {
let left_reg = program.alloc_register();
let right_reg = program.alloc_register();
build_limit_offset_expr(program, left_reg, left);
build_limit_offset_expr(program, right_reg, right);
match op {
Operator::Add => {
program.emit_insn(Insn::Add {
lhs: left_reg,
rhs: right_reg,
dest: r,
});
}
Operator::Subtract => {
program.emit_insn(Insn::Subtract {
lhs: left_reg,
rhs: right_reg,
dest: r,
});
}
Operator::Multiply => {
program.emit_insn(Insn::Multiply {
lhs: left_reg,
rhs: right_reg,
dest: r,
});
}
Operator::Divide => {
let zero_reg = program.alloc_register();
program.emit_int(0, zero_reg);
let ok_pc = program.allocate_label();
program.emit_insn(Insn::Ne {
lhs: right_reg,
rhs: zero_reg,
target_pc: ok_pc,
flags: CmpInsFlags::default().jump_if_null(),
collation: None,
});
program.emit_insn(Insn::Halt {
err_code: SQLITE_CONSTRAINT,
description: "divide by zero".into(),
});
program.resolve_label(ok_pc, program.offset());
program.emit_insn(Insn::Divide {
lhs: left_reg,
rhs: right_reg,
dest: r,
});
}
_ => {
program.emit_insn(Insn::Halt {
err_code: SQLITE_CONSTRAINT,
description: "unsupported operator in offset expr".into(),
});
}
}
}
_ => {
program.emit_insn(Insn::Halt {
err_code: SQLITE_CONSTRAINT,
description: "non-integer expression in offset".into(),
});
}
}
}
#[allow(clippy::borrowed_box)]
pub fn try_fold_expr_to_i64(expr: &Box<Expr>) -> Option<i64> {
match expr.as_ref() {

View File

@@ -8,8 +8,8 @@ use crate::schema::Table;
use crate::translate::optimizer::optimize_plan;
use crate::translate::plan::{Aggregate, GroupBy, Plan, ResultSetColumn, SelectPlan};
use crate::translate::planner::{
bind_column_references, break_predicate_at_and_boundaries, parse_from, parse_limit,
parse_where, resolve_aggregates,
bind_column_references, break_predicate_at_and_boundaries, parse_from, parse_where,
resolve_aggregates,
};
use crate::util::normalize_ident;
use crate::vdbe::builder::{ProgramBuilderOpts, TableRefIdCounter};
@@ -151,8 +151,7 @@ pub fn prepare_select_plan(
}
let (limit, offset) = select
.limit
.as_ref()
.map_or(Ok((None, None)), parse_limit)?;
.map_or((None, None), |l| (Some(l.expr), l.offset));
// FIXME: handle ORDER BY for compound selects
if !select.order_by.is_empty() {
@@ -622,7 +621,7 @@ fn prepare_one_select_plan(
plan.order_by = key;
// Parse the LIMIT/OFFSET clause
(plan.limit, plan.offset) = limit.as_ref().map_or(Ok((None, None)), parse_limit)?;
(plan.limit, plan.offset) = limit.map_or((None, None), |l| (Some(l.expr), l.offset));
// Return the unoptimized query plan
Ok(plan)

View File

@@ -21,8 +21,7 @@ use super::plan::{
ColumnUsedMask, IterationDirection, JoinedTable, Plan, ResultSetColumn, TableReferences,
UpdatePlan,
};
use super::planner::bind_column_references;
use super::planner::{parse_limit, parse_where};
use super::planner::{bind_column_references, parse_where};
/*
* Update is simple. By default we scan the table, and for each row, we check the WHERE
* clause. If it evaluates to true, we build the new record with the updated value and insert.
@@ -331,7 +330,10 @@ pub fn prepare_update_plan(
};
// Parse the LIMIT/OFFSET clause
let (limit, offset) = body.limit.as_ref().map_or(Ok((None, None)), parse_limit)?;
let (limit, offset) = body
.limit
.as_ref()
.map_or((None, None), |l| (Some(l.expr.clone()), l.offset.clone()));
// Check what indexes will need to be updated by checking set_clauses and see
// if a column is contained in an index.

View File

@@ -34,7 +34,7 @@ fn emit_values_when_single_row(
t_ctx: &TranslateCtx,
) -> Result<usize> {
let end_label = program.allocate_label();
emit_offset(program, plan, end_label, t_ctx.reg_offset);
emit_offset(program, plan, end_label, t_ctx.reg_offset, &t_ctx.resolver);
let first_row = &plan.values[0];
let row_len = first_row.len();
let start_reg = program.alloc_registers(row_len);
@@ -87,7 +87,7 @@ fn emit_toplevel_values(
});
let goto_label = program.allocate_label();
emit_offset(program, plan, goto_label, t_ctx.reg_offset);
emit_offset(program, plan, goto_label, t_ctx.reg_offset, &t_ctx.resolver);
let row_len = plan.values[0].len();
let copy_start_reg = program.alloc_registers(row_len);
for i in 0..row_len {