Skip to content

Commit b17b6ee

Browse files
sivukhinsivukhin
authored
adjust ALTER COLUMN parsing (#1520)
* add basic TCL tests for ALTER COLUMN libsql feature * propagate length of the new column definition directly from the parser - this will allow libsql to automatically handle comment and space characters appended to the column definition * small formatting fixes * cargo xtask build-bundled * add test against sqlite3 source in rust_suite * fix test in rust_suite
1 parent b988125 commit b17b6ee

File tree

6 files changed

+92
-22
lines changed

6 files changed

+92
-22
lines changed

‎libsql-ffi/bundled/src/sqlite3.c‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21663,7 +21663,7 @@ SQLITE_PRIVATE void sqlite3Reindex(Parse*, Token*, Token*);
2166321663
SQLITE_PRIVATE void sqlite3AlterFunctions(void);
2166421664
SQLITE_PRIVATE void sqlite3AlterRenameTable(Parse*, SrcList*, Token*);
2166521665
SQLITE_PRIVATE void sqlite3AlterRenameColumn(Parse*, SrcList*, Token*, Token*);
21666-
void libsqlAlterAlterColumn(Parse*, SrcList*, Token*, Token*);
21666+
void libsqlAlterAlterColumn(Parse*, SrcList*, Token*, Token*, int);
2166721667
SQLITE_PRIVATE int sqlite3GetToken(const unsigned char *, int *);
2166821668
SQLITE_PRIVATE void sqlite3NestedParse(Parse*, const char*, ...);
2166921669
SQLITE_PRIVATE void sqlite3ExpirePreparedStatements(sqlite3*, int);
@@ -116496,7 +116496,8 @@ void libsqlAlterAlterColumn(
116496116496
Parse *pParse, /* Parsing context */
116497116497
SrcList *pSrc, /* Table being altered. pSrc->nSrc==1 */
116498116498
Token *pOld, /* Name of column being changed */
116499-
Token *pNew /* New column declaration */
116499+
Token *pNew, /* New column declaration */
116500+
int nNewSqlLength /* New column declaration SQL string length (pNew.z is the start of the declaration) */
116500116501
){
116501116502
sqlite3 *db = pParse->db; /* Database connection */
116502116503
Table *pTab; /* Table being updated */
@@ -116554,9 +116555,7 @@ void libsqlAlterAlterColumn(
116554116555
}
116555116556
// NOTICE: this is the main difference in ALTER COLUMN compared to RENAME COLUMN,
116556116557
// we just take the whole new column declaration as it is.
116557-
// FIXME: the semicolon can also appear in the middle of the declaration when it's quoted,
116558-
// so we should check from the end.
116559-
pNew->n = sqlite3Strlen30(pNew->z);
116558+
pNew->n = nNewSqlLength;
116560116559
while (pNew->n > 0 && pNew->z[pNew->n - 1] == ';') pNew->n--;
116561116560
zNew = sqlite3DbStrNDup(db, pNew->z, pNew->n);
116562116561
if( !zNew ) goto exit_update_column;
@@ -117577,9 +117576,11 @@ static void renameColumnFunc(
117577117576
*/
117578117577
static void alterColumnFunc(
117579117578
sqlite3_context *context,
117580-
int NotUsed,
117579+
int argc,
117581117580
sqlite3_value **argv
117582117581
){
117582+
UNUSED_PARAMETER(argc);
117583+
117583117584
sqlite3 *db = sqlite3_context_db_handle(context);
117584117585
RenameCtx sCtx;
117585117586
const char *zSql = (const char*)sqlite3_value_text(argv[0]);
@@ -117602,7 +117603,6 @@ static void alterColumnFunc(
117602117603
sqlite3_xauth xAuth = db->xAuth;
117603117604
#endif
117604117605

117605-
UNUSED_PARAMETER(NotUsed);
117606117606
if( zSql==0 ) return;
117607117607
if( zTable==0 ) return;
117608117608
if( zNew==0 ) return;
@@ -117655,9 +117655,10 @@ static void alterColumnFunc(
117655117655
}
117656117656
} else {
117657117657
rc = SQLITE_ERROR;
117658-
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not ", IsView(sParse.pNewTable) ? "views" : "virtual tables");
117659-
goto alterColumnFunc_done; }
117660-
} else if (sParse.pNewIndex) {
117658+
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not %s", IsView(sParse.pNewTable) ? "views" : "virtual tables");
117659+
goto alterColumnFunc_done;
117660+
}
117661+
} else if( sParse.pNewIndex ){
117661117662
rc = SQLITE_ERROR;
117662117663
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not indexes");
117663117664
goto alterColumnFunc_done;
@@ -176759,7 +176760,8 @@ static YYACTIONTYPE yy_reduce(
176759176760
break;
176760176761
case 301: /* cmd ::= ALTER TABLE fullname ALTER COLUMNKW columnname TO columnname carglist */
176761176762
{
176762-
libsqlAlterAlterColumn(pParse, yymsp[-6].minor.yy203, &yymsp[-3].minor.yy0, &yymsp[-1].minor.yy0);
176763+
int definitionLength = (int)(pParse->sLastToken.z-yymsp[-1].minor.yy0.z) + pParse->sLastToken.n;
176764+
libsqlAlterAlterColumn(pParse, yymsp[-6].minor.yy203, &yymsp[-3].minor.yy0, &yymsp[-1].minor.yy0, definitionLength);
176763176765
}
176764176766
break;
176765176767
case 302: /* cmd ::= create_vtab */

‎libsql-sqlite3/src/alter.c‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -690,7 +690,8 @@ void libsqlAlterAlterColumn(
690690
Parse *pParse, /* Parsing context */
691691
SrcList *pSrc, /* Table being altered. pSrc->nSrc==1 */
692692
Token *pOld, /* Name of column being changed */
693-
Token *pNew /* New column declaration */
693+
Token *pNew, /* New column declaration */
694+
int nNewSqlLength /* New column declaration SQL string length (pNew.z is the start of the declaration) */
694695
){
695696
sqlite3 *db = pParse->db; /* Database connection */
696697
Table *pTab; /* Table being updated */
@@ -748,9 +749,7 @@ void libsqlAlterAlterColumn(
748749
}
749750
// NOTICE: this is the main difference in ALTER COLUMN compared to RENAME COLUMN,
750751
// we just take the whole new column declaration as it is.
751-
// FIXME: the semicolon can also appear in the middle of the declaration when it's quoted,
752-
// so we should check from the end.
753-
pNew->n = sqlite3Strlen30(pNew->z);
752+
pNew->n = nNewSqlLength;
754753
while (pNew->n > 0 && pNew->z[pNew->n - 1] == ';') pNew->n--;
755754
zNew = sqlite3DbStrNDup(db, pNew->z, pNew->n);
756755
if( !zNew ) goto exit_update_column;
@@ -1771,9 +1770,11 @@ static void renameColumnFunc(
17711770
*/
17721771
static void alterColumnFunc(
17731772
sqlite3_context *context,
1774-
int NotUsed,
1773+
int argc,
17751774
sqlite3_value **argv
17761775
){
1776+
UNUSED_PARAMETER(argc);
1777+
17771778
sqlite3 *db = sqlite3_context_db_handle(context);
17781779
RenameCtx sCtx;
17791780
const char *zSql = (const char*)sqlite3_value_text(argv[0]);
@@ -1796,7 +1797,6 @@ static void alterColumnFunc(
17961797
sqlite3_xauth xAuth = db->xAuth;
17971798
#endif
17981799

1799-
UNUSED_PARAMETER(NotUsed);
18001800
if( zSql==0 ) return;
18011801
if( zTable==0 ) return;
18021802
if( zNew==0 ) return;
@@ -1849,9 +1849,10 @@ static void alterColumnFunc(
18491849
}
18501850
} else {
18511851
rc = SQLITE_ERROR;
1852-
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not ", IsView(sParse.pNewTable) ? "views" : "virtual tables");
1853-
goto alterColumnFunc_done; }
1854-
} else if (sParse.pNewIndex) {
1852+
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not %s", IsView(sParse.pNewTable) ? "views" : "virtual tables");
1853+
goto alterColumnFunc_done;
1854+
}
1855+
} else if( sParse.pNewIndex ){
18551856
rc = SQLITE_ERROR;
18561857
sParse.zErrMsg = sqlite3MPrintf(sParse.db, "Only ordinary tables can be altered, not indexes");
18571858
goto alterColumnFunc_done;

‎libsql-sqlite3/src/parse.y‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1740,7 +1740,8 @@ cmd ::= ALTER TABLE fullname(X) RENAME kwcolumn_opt nm(Y) TO nm(Z). {
17401740
}
17411741

17421742
cmd ::= ALTER TABLE fullname(X) ALTER COLUMNKW columnname(Y) TO columnname(Z) carglist. {
1743-
libsqlAlterAlterColumn(pParse, X, &Y, &Z);
1743+
int definitionLength = (int)(pParse->sLastToken.z-Z.z) + pParse->sLastToken.n;
1744+
libsqlAlterAlterColumn(pParse, X, &Y, &Z, definitionLength);
17441745
}
17451746

17461747
kwcolumn_opt ::= .

‎libsql-sqlite3/src/sqliteInt.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5312,7 +5312,7 @@ void sqlite3Reindex(Parse*, Token*, Token*);
53125312
void sqlite3AlterFunctions(void);
53135313
void sqlite3AlterRenameTable(Parse*, SrcList*, Token*);
53145314
void sqlite3AlterRenameColumn(Parse*, SrcList*, Token*, Token*);
5315-
void libsqlAlterAlterColumn(Parse*, SrcList*, Token*, Token*);
5315+
void libsqlAlterAlterColumn(Parse*, SrcList*, Token*, Token*, int);
53165316
int sqlite3GetToken(const unsigned char *, int *);
53175317
void sqlite3NestedParse(Parse*, const char*, ...);
53185318
void sqlite3ExpirePreparedStatements(sqlite3*, int);

‎libsql-sqlite3/test/libsql_alter.test‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
set testdir [file dirname $argv0]
2+
3+
source $testdir/tester.tcl
4+
5+
# Test Organisation:
6+
# ------------------
7+
#
8+
# libsql_alter-ok.*: Test that ALTER COLUMN correctly modifies the CREATE TABLE sql.
9+
# libsql_alter-err.*: Test error messages.
10+
#
11+
12+
do_test libsql_alter-ok.1 {
13+
execsql {CREATE TABLE t1(x);}
14+
execsql {ALTER TABLE t1 ALTER COLUMN x TO x;}
15+
execsql {SELECT sql FROM sqlite_master WHERE tbl_name = 't1';}
16+
} {{CREATE TABLE t1(x)}}
17+
18+
do_test libsql_alter-ok.2 {
19+
execsql {CREATE TABLE t2(x);}
20+
execsql {ALTER TABLE t2 ALTER COLUMN x TO x INTEGER DEFAULT(-1);}
21+
execsql {SELECT sql FROM sqlite_master WHERE tbl_name = 't2';}
22+
} {{CREATE TABLE t2(x INTEGER DEFAULT(-1))}}
23+
24+
do_test libsql_alter-ok.3 {
25+
execsql {CREATE TABLE t3(x);}
26+
# NOTE: extra spaces in the end of ALTER COLUMN command
27+
execsql { ALTER TABLE t3 ALTER COLUMN x TO x INTEGER DEFAULT(-1); }
28+
execsql {SELECT sql FROM sqlite_master WHERE tbl_name = 't3';}
29+
} {{CREATE TABLE t3(x INTEGER DEFAULT(-1))}}
30+
31+
do_test libsql_alter-ok.4 {
32+
execsql {CREATE TABLE t4(x);}
33+
execsql { ALTER TABLE t4 ALTER COLUMN x TO x INTEGER DEFAULT(-1); -- explain alter command }
34+
execsql {SELECT sql FROM sqlite_master WHERE tbl_name = 't4';}
35+
} {{CREATE TABLE t4(x INTEGER DEFAULT(-1))}}
36+
37+
reset_db
38+
do_test libsql_alter-err.1 {
39+
execsql { CREATE TABLE t1(x); }
40+
catchsql { ALTER TABLE t1 ALTER COLUMN x TO x PRIMARY KEY; }
41+
} {1 {error in adding x PRIMARY KEY to t1: PRIMARY KEY constraint cannot be altered}}
42+
43+
do_test libsql_alter-err.2 {
44+
execsql { CREATE TABLE t2(x); }
45+
catchsql { ALTER TABLE t2 ALTER COLUMN x TO x INTEGER UNIQUE; }
46+
} {1 {error in adding x INTEGER UNIQUE to t2: UNIQUE constraint cannot be altered}}
47+
48+
do_test libsql_alter-err.3 {
49+
execsql { CREATE TABLE t3(x, y); }
50+
catchsql { ALTER TABLE t3 ALTER COLUMN y TO y GENERATED ALWAYS AS (2 * x) VIRTUAL; }
51+
} {1 {error in adding y GENERATED ALWAYS AS (2 * x) VIRTUAL to t3: GENERATED constraint cannot be altered}}
52+
53+
finish_test
54+

‎libsql-sqlite3/test/rust_suite/src/alter_column.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,15 @@ fn test_update_view_forbidden() {
184184
.execute("ALTER TABLE v ALTER COLUMN id TO id", ())
185185
.is_err());
186186
}
187+
188+
#[test]
189+
fn test_comment_in_the_end() {
190+
let conn = Connection::open_in_memory().unwrap();
191+
192+
conn.execute("CREATE TABLE t(id)", ()).unwrap();
193+
conn.execute(
194+
"ALTER TABLE t ALTER COLUMN id TO id CHECK(id < 5); -- explanation for alter command ",
195+
(),
196+
)
197+
.unwrap();
198+
}

0 commit comments

Comments
 (0)