From 72e8fa42f99399a1949d0d2967a0ac44dec6a52b Mon Sep 17 00:00:00 2001 From: drh Date: Wed, 28 Mar 2007 14:30:06 +0000 Subject: [PATCH] Correctly handle NULLs in IN operators. Ticket #2273. The changes in where.c and in the WhereLevel.aInLoop structure are not strictly necessary to fix this problem - they just make the code easier to read. Only the change in OP_Next/OP_Prev operator of vdbe.c is required. (CVS 3735) FossilOrigin-Name: 26348556d824c032851e409ac510cddb55c200bf --- manifest | 18 +++++------ manifest.uuid | 2 +- src/sqliteInt.h | 8 +++-- src/vdbe.c | 6 ++-- src/where.c | 83 ++++++++++++++++++++++++++++-------------------- test/where4.test | 57 +++++++++++++++++++++++++++++++-- 6 files changed, 123 insertions(+), 51 deletions(-) diff --git a/manifest b/manifest index bde82663e1..a7fdd54c5c 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Update\scomments\sin\ssqlite3.h.\s\sNo\schanges\sto\scode.\s(CVS\s3734) -D 2007-03-28T13:07:41 +C Correctly\shandle\sNULLs\sin\sIN\soperators.\s\sTicket\s#2273.\nThe\schanges\sin\swhere.c\sand\sin\sthe\sWhereLevel.aInLoop\sstructure\sare\nnot\sstrictly\snecessary\sto\sfix\sthis\sproblem\s-\sthey\sjust\smake\sthe\scode\neasier\sto\sread.\s\sOnly\sthe\schange\sin\sOP_Next/OP_Prev\soperator\sof\svdbe.c\nis\srequired.\s(CVS\s3735) +D 2007-03-28T14:30:07 F Makefile.in 1fe3d0b46e40fd684e1e61f8e8056cefed16de9f F Makefile.linux-gcc 2d8574d1ba75f129aba2019f0b959db380a90935 F README 9c4e2d6706bdcc3efdd773ce752a8cdab4f90028 @@ -98,7 +98,7 @@ F src/server.c 087b92a39d883e3fa113cae259d64e4c7438bc96 F src/shell.c 3ae4654560e91220a95738a73d135d91d937cda1 F src/sqlite.h.in 02d1159bc8f7387008df9766c79038fce8a9d3a7 F src/sqlite3ext.h 832e13de075d920e2c76584e2b7af1054bb212df -F src/sqliteInt.h 7a3d16cd517dfce73eeac10963275454d6421f82 +F src/sqliteInt.h ace601a40db30d667422c79a7db57e7dabb48038 F src/table.c 6d0da66dde26ee75614ed8f584a1996467088d06 F src/tclsqlite.c cd2b3b86ab07c0e0779f6c6e71e72c6c7dc1e704 F src/test1.c 439bba8da10fbc61c731019cf2894e6057578878 @@ -122,7 +122,7 @@ F src/update.c 876b3142b8db9ce6ddf8374a2ced65e576acabc7 F src/utf.c 67ecb1032bc0b42c105e88d65ef9d9f626eb0e1f F src/util.c 8e8180ee5597f2474c1da311ff3c464b6966c0f1 F src/vacuum.c 8bd895d29e7074e78d4e80f948e35ddc9cf2beef -F src/vdbe.c b68f3a86177fa4d10512a4e276a8961d5c1e2ccf +F src/vdbe.c 9fc4520600f364ec510e7c74733dc2b77430b92a F src/vdbe.h 0025259af1939fb264a545816c69e4b5b8d52691 F src/vdbeInt.h b2ca85ca8abfbba86e380b06c7673dc81c6784c3 F src/vdbeapi.c 6cff63a5b3a52af04b2bef0f7e27ed7ea6f85183 @@ -130,7 +130,7 @@ F src/vdbeaux.c 1a50dce809a2f5c2d32e8096166b59016dab1b45 F src/vdbefifo.c 3ca8049c561d5d67cbcb94dc909ae9bb68c0bf8f F src/vdbemem.c 58a8be2231b0b13e2e77098debe173e79d22f791 F src/vtab.c 7fbda947e28cbe7adb3ba752a76ca9ef29936750 -F src/where.c 0ef9acc43cc696b01a3f1019bc07c8d8e0849999 +F src/where.c df68bcbb07add015358a91b8412e2ea19dffdbc6 F tclinstaller.tcl 046e3624671962dc50f0481d7c25b38ef803eb42 F test/aggerror.test a867e273ef9e3d7919f03ef4f0e8c0d2767944f2 F test/all.test 60267b055e82de4fb8b841eabb014bc2f836a4eb @@ -367,7 +367,7 @@ F test/vtab_err.test 224cc80ad700797c48b9cd2c1e0bd7a8517d8609 F test/where.test 1d020f50c77f37b2dbab9766ca959e6e3278ecdb F test/where2.test 3249d426b3fc7a106713d784e1628307fc308d2e F test/where3.test 0a30fe9808b0fa01c46d0fcf4fac0bf6cf75bb30 -F test/where4.test 3fcf53c5ea7af1db3980b3293c2a45b56605f26a +F test/where4.test b68496500bff496e83e76ae4ffb493b99064eac6 F tool/diffdb.c 7524b1b5df217c20cd0431f6789851a4e0cb191b F tool/fragck.tcl 5265a95126abcf6ab357f7efa544787e5963f439 F tool/lemon.c d3dff81e31c459dd18784d024a759b7229f66297 @@ -442,7 +442,7 @@ F www/tclsqlite.tcl bb0d1357328a42b1993d78573e587c6dcbc964b9 F www/vdbe.tcl 87a31ace769f20d3627a64fa1fade7fed47b90d0 F www/version3.tcl 890248cf7b70e60c383b0e84d77d5132b3ead42b F www/whentouse.tcl 97e2b5cd296f7d8057e11f44427dea8a4c2db513 -P 0f7fdb022ca7c94f7d264192e18b6e2bd1e8cff4 -R b9541c43661a32a0d6639e1946203bc0 +P 1c2656fdf6176a7365db4e11f4bbf47721da72b4 +R ad4f14fb479cf3da73d29eb9f4dc5165 U drh -Z 44f123568a87e2be4a17739c7fa10306 +Z 203008a1e44e27fee31001b2c9831dfe diff --git a/manifest.uuid b/manifest.uuid index 9b9a9ba95e..10c399f53b 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -1c2656fdf6176a7365db4e11f4bbf47721da72b4 \ No newline at end of file +26348556d824c032851e409ac510cddb55c200bf \ No newline at end of file diff --git a/src/sqliteInt.h b/src/sqliteInt.h index fcf6c2d590..354e242e92 100644 --- a/src/sqliteInt.h +++ b/src/sqliteInt.h @@ -11,7 +11,7 @@ ************************************************************************* ** Internal interface definitions for SQLite. ** -** @(#) $Id: sqliteInt.h,v 1.545 2007/03/27 13:36:37 drh Exp $ +** @(#) $Id: sqliteInt.h,v 1.546 2007/03/28 14:30:07 drh Exp $ */ #ifndef _SQLITEINT_H_ #define _SQLITEINT_H_ @@ -1160,12 +1160,16 @@ struct WhereLevel { int iTabCur; /* The VDBE cursor used to access the table */ int iIdxCur; /* The VDBE cursor used to acesss pIdx */ int brk; /* Jump here to break out of the loop */ + int nxt; /* Jump here to start the next IN combination */ int cont; /* Jump here to continue with the next loop cycle */ int top; /* First instruction of interior of the loop */ int op, p1, p2; /* Opcode used to terminate the loop */ int nEq; /* Number of == or IN constraints on this loop */ int nIn; /* Number of IN operators constraining this loop */ - int *aInLoop; /* Loop terminators for IN operators */ + struct InLoop { + int iCur; /* The VDBE cursor used by this IN operator */ + int topAddr; /* Top of the IN loop */ + } *aInLoop; /* Information about each nested IN operator */ sqlite3_index_info *pBestIdx; /* Index information for this level */ /* The following field is really not part of the current level. But diff --git a/src/vdbe.c b/src/vdbe.c index 7385fe5597..ad0588a7c2 100644 --- a/src/vdbe.c +++ b/src/vdbe.c @@ -43,7 +43,7 @@ ** in this file for details. If in doubt, do not deviate from existing ** commenting and indentation practices when changing or adding code. ** -** $Id: vdbe.c,v 1.594 2007/03/27 14:44:51 drh Exp $ +** $Id: vdbe.c,v 1.595 2007/03/28 14:30:07 drh Exp $ */ #include "sqliteInt.h" #include "os.h" @@ -3677,7 +3677,9 @@ case OP_Next: { /* no-push */ CHECK_FOR_INTERRUPT; assert( pOp->p1>=0 && pOp->p1nCursor ); pC = p->apCsr[pOp->p1]; - assert( pC!=0 ); + if( pC==0 ){ + break; /* See ticket #2273 */ + } if( (pCrsr = pC->pCursor)!=0 ){ int res; if( pC->nullRow ){ diff --git a/src/where.c b/src/where.c index 031053a77f..8dffe625c8 100644 --- a/src/where.c +++ b/src/where.c @@ -16,7 +16,7 @@ ** so is applicable. Because this module is responsible for selecting ** indices, you might also think of this module as the "query optimizer". ** -** $Id: where.c,v 1.241 2007/03/27 13:36:37 drh Exp $ +** $Id: where.c,v 1.242 2007/03/28 14:30:09 drh Exp $ */ #include "sqliteInt.h" @@ -1687,7 +1687,6 @@ static void buildIndexProbe( static void codeEqualityTerm( Parse *pParse, /* The parsing context */ WhereTerm *pTerm, /* The term of the WHERE clause to be coded */ - int brk, /* Jump here to abandon the loop */ WhereLevel *pLevel /* When level of the FROM clause we are working on */ ){ Expr *pX = pTerm->pExpr; @@ -1699,21 +1698,25 @@ static void codeEqualityTerm( #ifndef SQLITE_OMIT_SUBQUERY }else{ int iTab; - int *aIn; + struct InLoop *pIn; assert( pX->op==TK_IN ); sqlite3CodeSubselect(pParse, pX); iTab = pX->iTable; sqlite3VdbeAddOp(v, OP_Rewind, iTab, 0); VdbeComment((v, "# %.*s", pX->span.n, pX->span.z)); + if( pLevel->nIn==0 ){ + pLevel->nxt = sqlite3VdbeMakeLabel(v); + } pLevel->nIn++; pLevel->aInLoop = sqliteReallocOrFree(pLevel->aInLoop, - sizeof(pLevel->aInLoop[0])*2*pLevel->nIn); - aIn = pLevel->aInLoop; - if( aIn ){ - aIn += pLevel->nIn*2 - 2; - aIn[0] = iTab; - aIn[1] = sqlite3VdbeAddOp(v, OP_Column, iTab, 0); + sizeof(pLevel->aInLoop[0])*pLevel->nIn); + pIn = pLevel->aInLoop; + if( pIn ){ + pIn += pLevel->nIn - 1; + pIn->iCur = iTab; + pIn->topAddr = sqlite3VdbeAddOp(v, OP_Column, iTab, 0); + sqlite3VdbeAddOp(v, OP_IsNull, -1, 0); }else{ pLevel->nIn = 0; } @@ -1749,8 +1752,7 @@ static void codeAllEqualityTerms( Parse *pParse, /* Parsing context */ WhereLevel *pLevel, /* Which nested loop of the FROM we are coding */ WhereClause *pWC, /* The WHERE clause */ - Bitmask notReady, /* Which parts of FROM have not yet been coded */ - int brk /* Jump here to end the loop */ + Bitmask notReady /* Which parts of FROM have not yet been coded */ ){ int nEq = pLevel->nEq; /* The number of == or IN constraints to code */ int termsInMem = 0; /* If true, store value in mem[] cells */ @@ -1779,9 +1781,9 @@ static void codeAllEqualityTerms( pTerm = findTerm(pWC, iCur, k, notReady, pLevel->flags, pIdx); if( pTerm==0 ) break; assert( (pTerm->flags & TERM_CODED)==0 ); - codeEqualityTerm(pParse, pTerm, brk, pLevel); - if( (pTerm->eOperator & WO_ISNULL)==0 ){ - sqlite3VdbeAddOp(v, OP_IsNull, termsInMem ? -1 : -(j+1), brk); + codeEqualityTerm(pParse, pTerm, pLevel); + if( (pTerm->eOperator & (WO_ISNULL|WO_IN))==0 ){ + sqlite3VdbeAddOp(v, OP_IsNull, termsInMem ? -1 : -(j+1), pLevel->brk); } if( termsInMem ){ sqlite3VdbeAddOp(v, OP_MemStore, pLevel->iMem+j+1, 1); @@ -2184,6 +2186,7 @@ WhereInfo *sqlite3WhereBegin( int j; int iCur = pTabItem->iCursor; /* The VDBE cursor for the table */ Index *pIdx; /* The index we will be using */ + int nxt; /* Where to jump to continue with the next IN case */ int iIdxCur; /* The VDBE cursor for the index */ int omitTable; /* True if we use the index only */ int bRev; /* True if we need to scan in reverse order */ @@ -2199,8 +2202,13 @@ WhereInfo *sqlite3WhereBegin( ** for the current loop. Jump to brk to break out of a loop. ** Jump to cont to go immediately to the next iteration of the ** loop. + ** + ** When there is an IN operator, we also have a "nxt" label that + ** means to continue with the next IN value combination. When + ** there are no IN operators in the constraints, the "nxt" label + ** is the same as "brk". */ - brk = pLevel->brk = sqlite3VdbeMakeLabel(v); + brk = pLevel->brk = pLevel->nxt = sqlite3VdbeMakeLabel(v); cont = pLevel->cont = sqlite3VdbeMakeLabel(v); /* If this is the right table of a LEFT OUTER JOIN, allocate and @@ -2266,9 +2274,10 @@ WhereInfo *sqlite3WhereBegin( assert( pTerm->pExpr!=0 ); assert( pTerm->leftCursor==iCur ); assert( omitTable==0 ); - codeEqualityTerm(pParse, pTerm, brk, pLevel); - sqlite3VdbeAddOp(v, OP_MustBeInt, 1, brk); - sqlite3VdbeAddOp(v, OP_NotExists, iCur, brk); + codeEqualityTerm(pParse, pTerm, pLevel); + nxt = pLevel->nxt; + sqlite3VdbeAddOp(v, OP_MustBeInt, 1, nxt); + sqlite3VdbeAddOp(v, OP_NotExists, iCur, nxt); VdbeComment((v, "pk")); pLevel->op = OP_Noop; }else if( pLevel->flags & WHERE_ROWID_RANGE ){ @@ -2347,7 +2356,7 @@ WhereInfo *sqlite3WhereBegin( /* Generate code to evaluate all constraint terms using == or IN ** and level the values of those terms on the stack. */ - codeAllEqualityTerms(pParse, pLevel, &wc, notReady, brk); + codeAllEqualityTerms(pParse, pLevel, &wc, notReady); /* Duplicate the equality term values because they will all be ** used twice: once to make the termination key and once to make the @@ -2378,6 +2387,7 @@ WhereInfo *sqlite3WhereBegin( ** 2002-Dec-04: On a reverse-order scan, the so-called "termination" ** key computed here really ends up being the start key. */ + nxt = pLevel->nxt; if( topLimit ){ Expr *pX; int k = pIdx->aiColumn[j]; @@ -2386,7 +2396,7 @@ WhereInfo *sqlite3WhereBegin( pX = pTerm->pExpr; assert( (pTerm->flags & TERM_CODED)==0 ); sqlite3ExprCode(pParse, pX->pRight); - sqlite3VdbeAddOp(v, OP_IsNull, -(nEq+1), brk); + sqlite3VdbeAddOp(v, OP_IsNull, -(nEq+1), nxt); topEq = pTerm->eOperator & (WO_LE|WO_GE); disableTerm(pLevel, pTerm); testOp = OP_IdxGE; @@ -2400,7 +2410,7 @@ WhereInfo *sqlite3WhereBegin( buildIndexProbe(v, nCol, pIdx); if( bRev ){ int op = topEq ? OP_MoveLe : OP_MoveLt; - sqlite3VdbeAddOp(v, op, iIdxCur, brk); + sqlite3VdbeAddOp(v, op, iIdxCur, nxt); }else{ sqlite3VdbeAddOp(v, OP_MemStore, pLevel->iMem, 1); } @@ -2425,7 +2435,7 @@ WhereInfo *sqlite3WhereBegin( pX = pTerm->pExpr; assert( (pTerm->flags & TERM_CODED)==0 ); sqlite3ExprCode(pParse, pX->pRight); - sqlite3VdbeAddOp(v, OP_IsNull, -(nEq+1), brk); + sqlite3VdbeAddOp(v, OP_IsNull, -(nEq+1), nxt); btmEq = pTerm->eOperator & (WO_LE|WO_GE); disableTerm(pLevel, pTerm); }else{ @@ -2440,7 +2450,7 @@ WhereInfo *sqlite3WhereBegin( testOp = OP_IdxLT; }else{ int op = btmEq ? OP_MoveGe : OP_MoveGt; - sqlite3VdbeAddOp(v, op, iIdxCur, brk); + sqlite3VdbeAddOp(v, op, iIdxCur, nxt); } }else if( bRev ){ testOp = OP_Noop; @@ -2455,7 +2465,7 @@ WhereInfo *sqlite3WhereBegin( start = sqlite3VdbeCurrentAddr(v); if( testOp!=OP_Noop ){ sqlite3VdbeAddOp(v, OP_MemLoad, pLevel->iMem, 0); - sqlite3VdbeAddOp(v, testOp, iIdxCur, brk); + sqlite3VdbeAddOp(v, testOp, iIdxCur, nxt); if( (topEq && !bRev) || (!btmEq && bRev) ){ sqlite3VdbeChangeP3(v, -1, "+", P3_STATIC); } @@ -2484,7 +2494,8 @@ WhereInfo *sqlite3WhereBegin( /* Generate code to evaluate all constraint terms using == or IN ** and leave the values of those terms on the stack. */ - codeAllEqualityTerms(pParse, pLevel, &wc, notReady, brk); + codeAllEqualityTerms(pParse, pLevel, &wc, notReady); + nxt = pLevel->nxt; /* Generate a single key that will be used to both start and terminate ** the search @@ -2493,21 +2504,21 @@ WhereInfo *sqlite3WhereBegin( sqlite3VdbeAddOp(v, OP_MemStore, pLevel->iMem, 0); /* Generate code (1) to move to the first matching element of the table. - ** Then generate code (2) that jumps to "brk" after the cursor is past + ** Then generate code (2) that jumps to "nxt" after the cursor is past ** the last matching element of the table. The code (1) is executed ** once to initialize the search, the code (2) is executed before each ** iteration of the scan to see if the scan has finished. */ if( bRev ){ /* Scan in reverse order */ - sqlite3VdbeAddOp(v, OP_MoveLe, iIdxCur, brk); + sqlite3VdbeAddOp(v, OP_MoveLe, iIdxCur, nxt); start = sqlite3VdbeAddOp(v, OP_MemLoad, pLevel->iMem, 0); - sqlite3VdbeAddOp(v, OP_IdxLT, iIdxCur, brk); + sqlite3VdbeAddOp(v, OP_IdxLT, iIdxCur, nxt); pLevel->op = OP_Prev; }else{ /* Scan in the forward order */ - sqlite3VdbeAddOp(v, OP_MoveGe, iIdxCur, brk); + sqlite3VdbeAddOp(v, OP_MoveGe, iIdxCur, nxt); start = sqlite3VdbeAddOp(v, OP_MemLoad, pLevel->iMem, 0); - sqlite3VdbeOp3(v, OP_IdxGE, iIdxCur, brk, "+", P3_STATIC); + sqlite3VdbeOp3(v, OP_IdxGE, iIdxCur, nxt, "+", P3_STATIC); pLevel->op = OP_Next; } if( !omitTable ){ @@ -2640,16 +2651,18 @@ void sqlite3WhereEnd(WhereInfo *pWInfo){ if( pLevel->op!=OP_Noop ){ sqlite3VdbeAddOp(v, pLevel->op, pLevel->p1, pLevel->p2); } - sqlite3VdbeResolveLabel(v, pLevel->brk); if( pLevel->nIn ){ - int *a; + struct InLoop *pIn; int j; - for(j=pLevel->nIn, a=&pLevel->aInLoop[j*2-2]; j>0; j--, a-=2){ - sqlite3VdbeAddOp(v, OP_Next, a[0], a[1]); - sqlite3VdbeJumpHere(v, a[1]-1); + sqlite3VdbeResolveLabel(v, pLevel->nxt); + for(j=pLevel->nIn, pIn=&pLevel->aInLoop[j-1]; j>0; j--, pIn--){ + sqlite3VdbeJumpHere(v, pIn->topAddr+1); + sqlite3VdbeAddOp(v, OP_Next, pIn->iCur, pIn->topAddr); + sqlite3VdbeJumpHere(v, pIn->topAddr-1); } sqliteFree(pLevel->aInLoop); } + sqlite3VdbeResolveLabel(v, pLevel->brk); if( pLevel->iLeftJoin ){ int addr; addr = sqlite3VdbeAddOp(v, OP_IfMemPos, pLevel->iLeftJoin, 0); diff --git a/test/where4.test b/test/where4.test index fc616482b0..f9b71d072d 100644 --- a/test/where4.test +++ b/test/where4.test @@ -15,7 +15,7 @@ # that IS NULL phrases are correctly optimized. But you can never # have too many tests, so some other tests are thrown in as well. # -# $Id: where4.test,v 1.2 2007/01/25 16:56:08 drh Exp $ +# $Id: where4.test,v 1.3 2007/03/28 14:30:09 drh Exp $ set testdir [file dirname $argv0] source $testdir/tester.tcl @@ -173,7 +173,60 @@ do_test where4-4.4 { WHERE t1.col1 IS NULL; } } {} - + +# Ticket #2273. Problems with IN operators and NULLs. +# +do_test where4-5.1 { + execsql { + CREATE TABLE t4(x,y,z,PRIMARY KEY(x,y)); + } + execsql { + SELECT * + FROM t2 LEFT JOIN t4 b1 + LEFT JOIN t4 b2 ON b2.x=b1.x AND b2.y IN (b1.y); + } +} {1 {} {} {} {} {} {} 2 {} {} {} {} {} {} 3 {} {} {} {} {} {}} +do_test where4-5.2 { + execsql { + INSERT INTO t4 VALUES(1,1,11); + INSERT INTO t4 VALUES(1,2,12); + INSERT INTO t4 VALUES(1,3,13); + INSERT INTO t4 VALUES(2,2,22); + SELECT rowid FROM t4 WHERE x IN (1,9,2,5) AND y IN (1,3,NULL,2) AND z!=13; + } +} {1 2 4} +do_test where4-5.3 { + execsql { + SELECT rowid FROM t4 WHERE x IN (1,9,NULL,2) AND y IN (1,3,2) AND z!=13; + } +} {1 2 4} +do_test where4-6.1 { + execsql { + CREATE TABLE t5(a,b,c,d,e,f,UNIQUE(a,b,c,d,e,f)); + INSERT INTO t5 VALUES(1,1,1,1,1,11111); + INSERT INTO t5 VALUES(2,2,2,2,2,22222); + INSERT INTO t5 VALUES(1,2,3,4,5,12345); + INSERT INTO t5 VALUES(2,3,4,5,6,23456); + } + execsql { + SELECT rowid FROM t5 + WHERE a IN (1,9,2) AND b=2 AND c IN (1,2,3,4) AND d>0 + } +} {3 2} +do_test where4-6.2 { + execsql { + SELECT rowid FROM t5 + WHERE a IN (1,NULL,2) AND b=2 AND c IN (1,2,3,4) AND d>0 + } +} {3 2} +do_test where4-7.1 { + execsql { + CREATE TABLE t6(y,z,PRIMARY KEY(y,z)); + } + execsql { + SELECT * FROM t6 WHERE y=NULL AND z IN ('hello'); + } +} {} integrity_check {where4-99.0}