Fix panic in case when table has row with rowid equals to `-1`
(`=u64::max`)
```sql
limbo> CREATE TABLE t(x INTEGER PRIMARY KEY)
limbo> INSERT INTO t VALUES (-1)
limbo> INSERT INTO t VALUES (NULL);
thread 'main' panicked at core/vdbe/mod.rs:2499:21:
attempt to add with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
Reviewed-by: Pere Diaz Bou <pere-altea@homail.com>
Closes#868
We transform all JOIN conditions into WHERE clause terms in the query
planner. The JoinAwareConditionExpr name tries to make that point, but I
think it makes things more confusing. Let's call it WhereTerm (suggested
by Jussi).
## Simplify bookkeeping of referenced tables in the query planner
This PR refactors the way we track referenced tables and associated
planner operations related to them (scans, index searches, etc).
## The problem with what we currently have:
- We have a tree data structure called `SourceOperator` which is either
a `Join`, `Scan`, `Search`, `Subquery` or `Nothing`.
```rust
/**
A SourceOperator is a Node in the query plan that reads data from a table.
*/
#[derive(Clone, Debug)]
pub enum SourceOperator {
// Join operator
// This operator is used to join two source operators.
// It takes a left and right source operator, a list of predicates to evaluate,
// and a boolean indicating whether it is an outer join.
Join {
id: usize,
left: Box<SourceOperator>,
right: Box<SourceOperator>,
predicates: Option<Vec<ast::Expr>>,
outer: bool,
using: Option<ast::DistinctNames>,
},
// Scan operator
// This operator is used to scan a table.
// It takes a table to scan and an optional list of predicates to evaluate.
// The predicates are used to filter rows from the table.
// e.g. SELECT * FROM t1 WHERE t1.foo = 5
// The iter_dir are uset to indicate the direction of the iterator.
// The use of Option for iter_dir is aimed at implementing a conservative optimization strategy: it only pushes
// iter_dir down to Scan when iter_dir is None, to prevent potential result set errors caused by multiple
// assignments. for more detailed discussions, please refer to https://github.com/penberg/limbo/pull/376
Scan {
id: usize,
table_reference: TableReference,
predicates: Option<Vec<ast::Expr>>,
iter_dir: Option<IterationDirection>,
},
// Search operator
// This operator is used to search for a row in a table using an index
// (i.e. a primary key or a secondary index)
Search {
id: usize,
table_reference: TableReference,
search: Search,
predicates: Option<Vec<ast::Expr>>,
},
Subquery {
id: usize,
table_reference: TableReference,
plan: Box<SelectPlan>,
predicates: Option<Vec<ast::Expr>>,
},
// Nothing operator
// This operator is used to represent an empty query.
// e.g. SELECT * from foo WHERE 0 will eventually be optimized to Nothing.
Nothing {
id: usize,
},
}
```
- Logically joins are a tree, but this is at least marginally bad for
performance because each `Join` has two boxed child operators, and so
e.g. for a 3-table query you have, for example, 3 `Scan` nodes and then
2 `Join` nodes.
- There are other bigger problems too, though, related to code
structure. We have been carrying around a separate vector of
`referenced_tables` that columns can refer to by index:
```rust
/// A query plan has a list of TableReference objects, each of which represents a table or subquery.
#[derive(Clone, Debug)]
pub struct TableReference {
/// Table object, which contains metadata about the table, e.g. columns.
pub table: Table,
/// The name of the table as referred to in the query, either the literal name or an alias e.g. "users" or "u"
pub table_identifier: String,
/// The index of this reference in the list of TableReference objects in the query plan
/// The reference at index 0 is the first table in the FROM clause, the reference at index 1 is the second table in the FROM clause, etc.
/// So, the index is relevant for determining when predicates (WHERE, ON filters etc.) should be evaluated.
pub table_index: usize,
/// The type of the table reference, either BTreeTable or Subquery
pub reference_type: TableReferenceType,
}
```
- `referenced_tables` is used because SQLite joins are an `n^tables_len`
nested loop, and we need to figure out during which loop to evaluate a
condition expression. A lot of plumbing in the current code exists for
this, e.g. "pushing predicates" in `optimizer` even though "predicate
pushdown" as a query planner concept is an _optimization_, but in our
current system the "pushdown" is really a _necessity_ to move the
condition expressions to the correct `SourceOperator::predicates` vector
so that they are evaluated at the right point.
- `referenced_tables` is also used to map identifiers in the query to
the correct table, e.g. 'foo' `SELECT foo FROM users` becomes an
`ast::Expr::Column { table: 0, .. }` if `users` is the first table in
`referenced_tables`.
In addition to this, we ALSO had a `TableReferenceType` separately for
checking whether the upper-level query is reading from a BTree table or
a Subquery.
```rust
/// The type of the table reference, either BTreeTable or Subquery
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum TableReferenceType {
/// A BTreeTable is a table that is stored on disk in a B-tree index.
BTreeTable,
/// A subquery.
Subquery {
/// The index of the first register in the query plan that contains the result columns of the subquery.
result_columns_start_reg: usize,
},
}
```
...even though we already have an `Operator::Subquery` that should be
able to encode this information, but doesn't, because it's a tree and we
refer to things by index in `referenced_tables`.
### Why this is especially stupid
`SourceOperator` and `TableReference` are basically just two
representations of the same thing, one in tree format and another in
vector format. `SourceOperator` even carries around its own copy of
`TableReference`, even though we ALSO have `referenced_tables:
Vec<TableReference>` 🤡
Note that I'm allowed to call the existing code stupid because I wrote
it.
## What we can do instead
Basically, we can just fold the concerns from `SourceOperator` into
`TableReference` and have a list of those in the query plan, one per
table, in loop order (outermost loop is 0, and so on).
Funnily enough, when Limbo had very very few features we used to have a
Vec of LoopInfos similarly, obviously with a lot less information than
now, but for SQLite it's probably the right abstraction. :)
```rust
pub struct SelectPlan {
/// List of table references in loop order, outermost first.
pub table_references: Vec<TableReference>,
...etc...
}
/// A table reference in the query plan.
/// For example, SELECT * FROM users u JOIN products p JOIN (SELECT * FROM users) sub
/// has three table references:
/// 1. operation=Scan, table=users, table_identifier=u, reference_type=BTreeTable, join_info=None
/// 2. operation=Scan, table=products, table_identifier=p, reference_type=BTreeTable, join_info=Some(JoinInfo { outer: false, using: None }),
/// 3. operation=Subquery, table=users, table_identifier=sub, reference_type=Subquery, join_info=None
#[derive(Debug, Clone)]
pub struct TableReference {
/// The operation that this table reference performs.
pub op: Operation,
/// Table object, which contains metadata about the table, e.g. columns.
pub table: Table,
/// The name of the table as referred to in the query, either the literal name or an alias e.g. "users" or "u"
pub identifier: String,
/// The join info for this table reference if it is the right side of a join (which all except the first table reference have)
pub join_info: Option<JoinInfo>,
}
```
And we keep the "operation" part from the "operator", but in a simple
form:
```rust
pub enum Operation {
Scan {
iter_dir: Option<IterationDirection>,
},
Search(Search),
Subquery {
plan: Box<SelectPlan>,
result_columns_start_reg: usize
},
}
```
Now we don't need to carry around both the operator tree and
`Vec<TableReference>`, because they are the same thing. If something
refers to the `n'th table`, it is just `plan.table_references[n]`.
We also don't need to recurse through the operator tree and usually we
can just loop from outermost table to innermost table.
---
### Handling the "where to evaluate a condition expression" problem
You can see I've also removed the `predicates` vector from `Scan` and
friends. Previously each `SourceOperator` had a vector of `predicates`
so that we knew at which loop depth to evaluate a condition. Now we
align more with what SQLite does -- it puts all the conditions, even the
join conditions, in the `WHERE` clause and adds extra metadata to them:
```rust
/// In a query plan, WHERE clause conditions and JOIN conditions are all folded into a vector of JoinAwareConditionExpr.
/// This is done so that we can evaluate the conditions at the correct loop depth.
/// We also need to keep track of whether the condition came from an OUTER JOIN. Take this example:
/// SELECT * FROM users u LEFT JOIN products p ON u.id = 5.
/// Even though the condition only refers to 'u', we CANNOT evaluate it at the users loop, because we need to emit NULL
/// values for the columns of 'p', for EVERY row in 'u', instead of completely skipping any rows in 'u' where the condition is false.
#[derive(Debug, Clone)]
pub struct JoinAwareConditionExpr {
/// The original condition expression.
pub expr: ast::Expr,
/// Is this condition originally from an OUTER JOIN?
/// If so, we need to evaluate it at the loop of the right table in that JOIN,
/// regardless of which tables it references.
/// We also cannot e.g. short circuit the entire query in the optimizer if the condition is statically false.
pub from_outer_join: bool,
/// The loop index where to evaluate the condition.
/// For example, in `SELECT * FROM u JOIN p WHERE u.id = 5`, the condition can already be evaluated at the first loop (idx 0),
/// because that is the rightmost table that it references.
pub eval_at_loop: usize,
}
```
### Final notes
I've been wanting to make this refactor for a long time now, but the
last straw was when I was making a PR trying to reduce some of the
massive amount of allocations happening in the read path currently, and
I got stuck because of this Operator + referenced_tables shit getting in
the way constantly. So I decided I wanted to get it over with now.
The PR is again very big, but I've artificially split it up into commits
that don't individually compile but at least separate the changes a bit
for you, dear reader.
Closes#853
We don't need `push_predicates()` because that never REALLY was a predicate
pushdown optimization -- it just pushed WHERE clause condition expressions
into the correct SourceOperator nodes in the tree.
Now that we don't have a SourceOperator tree anymore and we keep the conditions
in the WHERE clause instead, we don't need to "push" anything anymore. Leaves
room for ACTUAL predicate pushdown optimizations later :)
We also don't need any weird bitmask stuff anymore, and perhaps we never did,
to determine where conditions should be evaluated.
Now that we do not have a tree of SourceOperators but rather
a Vec of TableReferences, we can just use loops instead of
recursion for handling the main query loop.
- use new TableReference and JoinAwareConditionExpr
- add utilities for determining at which loop depth a
WHERE condition should be evaluated, now that "operators"
do not carry condition expressions inside them anymore.
- Get rid of SourceOperator tree
- Make plan have a Vec of TableReference, and TableReference now
contains the information from the old SourceOperator.
- Remove `predicates` (conditions) from Table References -- put
everything in the WHERE clause like SQLite, and attach metadata
to the where clause expressions with JoinAwareConditionExpr struct.
- Refactor select_star() to be simpler now that we use a vec, not a tree
This PR adjust emitted instructions for expressions which include `IS` /
`IS NOT` operators (support for them in the conditions were added in the
#847)
Reviewed-by: Glauber Costa (@glommer)
Closes#857
This PR remove manual null-comparison optimization (introduced in #847)
which replace `Eq`/`Ne` instructions with explicit `IsNull`/`NotNull`.
There are few factors for this change:
1. Before, manual optimization were incorrect because it ignored
`jump_if_condition_is_true` flag which is important to properly build
logical condition evaluation
2. Manual optimization covered all scenarios in test cases and scenarios
when both sides are non trivial expressions were not covered by tests
3. Limbo already mark literals in the initial emitted bytecode as
constants and will evaluate and store them only once - so performance
difference from manual optimization seems very minor to me (but I am
wrong with high probability)
4. I think (but again, I am wrong with high probability) that such
replacement can be done in the generic optimizator layer instead of
manually encode them in the first emit phase
Fixes#850
Reviewed-by: Glauber Costa (@glommer)
Closes#856
The main difference between = and != is how null values are handled.
SQLite passes a flag "NULLEQ" to Eq and Ne to disambiguate that.
In the presence of that flag, NULL = NULL.
Some prep work is done to make sure we can pass a flag instead of a
boolean to Eq and Ne. I looked into the bitflags crate but got a bit
scared with the list of dependencies.
Warning:
The following query produces a different result for Limbo:
```
select * from demo where value is null or id == 2;
```
I strongly suspect the issue is with the OR implementation, though. The
bytecode generated is quite different.
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#847
This seems to really be just an alias for IS:
"The IS NOT DISTINCT FROM operator is an alternative spelling for the IS
operator. Likewise, the IS DISTINCT FROM operator means the same thing
as IS NOT. Standard SQL does not support the compact IS and IS NOT
notation. Those compact forms are an SQLite extension. You have to use
the prolix and much less readable IS NOT DISTINCT FROM and IS DISTINCT
FROM operators on other SQL database engines."
Improvements:
- Use [rust-overlay](https://github.com/oxalica/rust-overlay), better
maintained than fenix and allows for:
- Use `rust-toolchain.toml` as the source of truth for the current rust
version, instead of tracking with stable. Preventing conflicting
versions with non-nix users.
- Add flake checks, could be useful for CI in the future, together with
crane and cachix.
- Add package, allow people to add limbo as a regular nix package. Now
we can `nix build .#`, `nix run .#` and `nix shell .#` (this one adds
`limbo` to the current `PATH`)
- Use [new `apple-sdk` pattern](https://discourse.nixos.org/t/the-
darwin-sdks-have-been-updated/55295), no need to declare each framework
now.
Closes#835
This PR continues work on the Go bindings.
- Register all symbols from the library at load time to prevent any
repeated `dlsym` calls.
- Add locks to prevent multiple concurrent FFI calls to functions that
act on the same state.
- Adds documentation/example in the go module `README`.
- Fixes memory access issue causing segfault due to passing pointer to
array of strings, that is difficult to work with in Go without the right
primitives. In place, simply return the amount of ResultColumns and Go
can provide the index to receive the column name, similar to
`rowsGetValue`
On next limbo release, I'll add the example to the main `README` next to
the other language examples. Until then, `go get
github.com/tursodatabase/limbo` will not work so the example will remain
in the bindings readme.
Closes#845
Instead of always having the caller specify all instructions, this
work introduces convenience functions into the program builder,
making the code a lot cleaner.
Draft for now, as this is done on top of #841
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#844
The main difference between = and != is how null values are handled.
SQLite passes a flag "NULLEQ" to Eq and Ne to disambiguate that.
In the presence of that flag, NULL = NULL.
Some prep work is done to make sure we can pass a flag instead of a
boolean to Eq and Ne. I looked into the bitflags crate but got a bit
scared with the list of dependencies.
This PR implements Noop. I really don't know what else to say. This
bytecode according to sqlite does: _Do nothing. Continue downward to the
next opcode._ I advanced the program counter to account for continuing
to the next instruction.
Closes#795
Instead of always having the caller specify all instructions, this
work introduces convenience functions into the program builder,
making the code a lot cleaner.
The error is due to comparing the PRIMARY KEY's name to INTEGER when in
it was all in lowercase. This was causing `needs_auto_index` to be set
to `true`.
After the fix:
```
/limbo /tmp/sc2-limbo.db
Limbo v0.0.13
Enter ".help" for usage hints.
limbo> CREATE TABLE temp (t1 integer, primary key (t1));
hexdump -s 28 -n 4 /tmp/sc2-limbo.db
000001c 0000 0200 -- matches SQLite
0000020
```
Closes https://github.com/tursodatabase/limbo/issues/824
Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>
Closes#830
