Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add regression tests for sqlite explain #2180

Closed
wants to merge 9 commits into from

Conversation

0rvar
Copy link

@0rvar 0rvar commented Oct 30, 2022

Original: Fix SQLx+Sqlite incorrectly inferring column as nullable when using ORDER BY and LIMIT in the same query

Status: added a failing test, reading through the opcode state machine in explain.
Tips and ideas are appreciated.

Related:

Semi-related:

It seems the original issue was already fixed in 0.7-dev.
I changed this PR to add more tests for the issues in 0.6 if they are of interest - otherwise, feel free to just close this.

Wopple and others added 8 commits September 14, 2022 17:34
Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

Co-authored-by: Daniel Tashjian <daniel@ecomedes.com>
* add failing test cases for update/delete return into

* fix regression in null tracking by improving tracking of cursor empty/full state

* add failing test case for order by column types

* Add support for SorterOpen,SorterInsert,SorterData

* add failing test case for unions

* fix range copy/move implementation

* fix wrong copy/move range

* remove calls to dbg!
…chbadge#1946)

* add instruction, register, and cursor state memorization

* fix: fixed formating
* feat: Add set_connect_options method to Pool

This allows external updates of the ConnectionOptions used when a new
connection needs to be opened for the pool.  The primary use case
is to support dynamically updated (read: rotated) credentials used
by systems like AWS RDS.

* Use Arc wrapper for ConnectOptions to reduce lock contention

* sqlite fix

* Use direct assignment instead of mem::swap

Co-authored-by: Austin Bonander <austin.bonander@gmail.com>

Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
* AggValue and ROW_NUMBER()

* Some more functions

* cargo fmt
@0rvar
Copy link
Author

0rvar commented Oct 30, 2022

Some opcodes for reference
sqlite> explain select accounts.name from accounts;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     8     0                    0   Start at 8
1     OpenRead       0     6     0     2              0   root=6 iDb=0; accounts
2     Explain        2     0     0     SCAN accounts  0
3     Rewind         0     7     0                    0
4       Column         0     1     1                    0   r[1]=accounts.name
5       ResultRow      1     1     0                    0   output=r[1]
6     Next           0     4     0                    1
7     Halt           0     0     0                    0
8     Transaction    0     0     20    0              1   usesStmtJournal=0
9     Goto           0     1     0                    0

==========================================================================================

sqlite> explain select accounts.name from accounts ORDER BY accounts.name;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     16    0                    0   Start at 16
1     SorterOpen     1     3     0     k(1,B)         0
2     OpenRead       0     6     0     2              0   root=6 iDb=0; accounts
3     Explain        3     0     0     SCAN accounts  0
4     Rewind         0     9     0                    0
5       Column         0     1     1                    0   r[1]=accounts.name
6       MakeRecord     1     1     3                    0   r[3]=mkrec(r[1])
7       SorterInsert   1     3     1     1              0   key=r[3]
8     Next           0     5     0                    1
9     OpenPseudo     2     4     3                    0   3 columns in r[4]
10    SorterSort     1     15    0                    0
11      SorterData     1     4     2                    0   r[4]=data
12      Column         2     0     2                    0   r[2]=accounts.name
13      ResultRow      2     1     0                    0   output=r[2]
14    SorterNext     1     11    0                    0
15    Halt           0     0     0                    0
16    Transaction    0     0     20    0              1   usesStmtJournal=0
17    Goto           0     1     0                    0

==========================================================================================

sqlite> explain select accounts.name from accounts LIMIT 1;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     10    0                    0   Start at 10
1     Integer        1     1     0                    0   r[1]=1; LIMIT counter
2     OpenRead       0     6     0     2              0   root=6 iDb=0; accounts
3     Explain        3     0     0     SCAN accounts  0
4     Rewind         0     9     0                    0
5       Column         0     1     2                    0   r[2]=accounts.name
6       ResultRow      2     1     0                    0   output=r[2]
7       DecrJumpZero   1     9     0                    0   if (--r[1])==0 goto 9
8     Next           0     5     0                    1
9     Halt           0     0     0                    0
10    Transaction    0     0     20    0              1   usesStmtJournal=0
11    Goto           0     1     0                    0

==========================================================================================

sqlite> explain select accounts.name from accounts ORDER BY accounts.name LIMIT 1;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     20    0                    0   Start at 20
1     OpenEphemeral  1     3     0     k(1,B)         0   nColumn=3
2     Integer        1     1     0                    0   r[1]=1; LIMIT counter
3     OpenRead       0     6     0     2              0   root=6 iDb=0; accounts
4     Explain        4     0     0     SCAN accounts  0
5     Rewind         0     15    0                    0
6       Column         0     1     2                    0   r[2]=accounts.name
7       Sequence       1     3     0                    0   r[3]=cursor[1].ctr++
8       IfNotZero      1     12    0                    0   if r[1]!=0 then r[1]--, goto 12
9       Last           1     0     0                    0
10      IdxLE          1     14    2     1              0   key=r[2]
11      Delete         1     0     0                    0
12      MakeRecord     2     2     5                    0   r[5]=mkrec(r[2..3])
13      IdxInsert      1     5     2     2              0   key=r[5]
14    Next           0     6     0                    1
15    Sort           1     19    0                    0
16      Column         1     0     4                    0   r[4]=accounts.name
17      ResultRow      4     1     0                    0   output=r[4]
18    Next           1     16    0                    0
19    Halt           0     0     0                    0
20    Transaction    0     0     20    0              1   usesStmtJournal=0
21    Goto           0     1     0                    0

@tyrelr
Copy link
Contributor

tyrelr commented Oct 30, 2022

Just as a heads up, you may want to base your pull request off of the "0.7-dev" branch instead of main. Changes to type-inference are delayed until the next minor version, rather than a bugfix release. And there is at least one fix for 'order by' type inference in that branch already.

@0rvar
Copy link
Author

0rvar commented Oct 30, 2022

@tyrelr well, it seems to be fixed in 0.7-dev. That's great news.
I'll add some more tests from the related issues I linked, maybe this PR will just be about adding some more tests in the end.
Thanks for pointing this out.

@0rvar 0rvar changed the base branch from main to 0.7-dev October 30, 2022 18:44
@0rvar 0rvar force-pushed the fix-sqlite-order-and-limit branch from 1f285a7 to 3961dc3 Compare October 30, 2022 18:45
@0rvar 0rvar marked this pull request as ready for review October 30, 2022 18:49
@0rvar 0rvar changed the title Fix SQLx+Sqlite incorrectly inferring column as nullable when using ORDER BY and LIMIT in the same query Add regression tests for sqlite explain Oct 31, 2022
@abonander
Copy link
Collaborator

@0rvar can you rebase?

@abonander abonander deleted the branch launchbadge:0.7-dev February 21, 2023 21:25
@abonander abonander closed this Feb 21, 2023
@0rvar
Copy link
Author

0rvar commented Mar 6, 2023

@abonander I now have capacity to rebase this, if you're still interested. Let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants