Skip to content

Commit

Permalink
Merge pull request #5193 from rouault/fix_qgis_47012
Browse files Browse the repository at this point in the history 
GPKG/SQLite: use ALTER TABLE ... RENAME/DROP COLUMN when possible ...
  • Loading branch information
@rouault
rouault committed Jan 30, 2022
2 parents 494444f + 55d3597 commit 4f2cb6f
Show file tree
Hide file tree
Showing 9 changed files with 487 additions and 208 deletions.
41 changes: 40 additions & 1 deletion autotest/ogr/ogr_gpkg.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2569,6 +2569,18 @@ def test_ogr_gpkg_34():
assert gdal.GetLastErrorMsg() != ''
gdal.ErrorReset()
ds.ExecuteSQL('ALTER TABLE "weird\'layer""name" RENAME TO "weird2\'layer""name"')
ds = None

ds = ogr.Open(dbname, update=1)
ds.ExecuteSQL('ALTER TABLE "weird2\'layer""name" RENAME COLUMN "foo" TO "bar"')
assert gdal.GetLastErrorMsg() == ''
lyr = ds.GetLayerByName(new_layer_name)
assert lyr.GetLayerDefn().GetFieldIndex('bar') >= 0

ds.ExecuteSQL('ALTER TABLE "weird2\'layer""name" DROP COLUMN "bar"')
assert gdal.GetLastErrorMsg() == ''
assert lyr.GetLayerDefn().GetFieldIndex('bar') < 0

ds.ExecuteSQL('VACUUM')
ds = None

Expand Down Expand Up @@ -2832,8 +2844,26 @@ def test_ogr_gpkg_36():

new_field_defn = ogr.FieldDefn('bar', ogr.OFTReal)
new_field_defn.SetSubType(ogr.OFSTFloat32)
new_field_defn.SetWidth(10)
new_field_defn.SetDefault('5')

# Schema only change
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0

# Full table rewrite
new_field_defn.SetNullable(False)
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0

# Full table rewrite
new_field_defn.SetUnique(True)
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0

# Violation of not-null constraint
new_field_defn = ogr.FieldDefn('baz', ogr.OFTString)
new_field_defn.SetNullable(False)
with gdaltest.error_handler():
assert lyr.AlterFieldDefn(1 , new_field_defn, ogr.ALTER_ALL_FLAG) != 0

lyr.ResetReading()
f = lyr.GetNextFeature()
if f.GetFID() != 10 or f['bar'] != 10.5 or \
Expand All @@ -2842,6 +2872,13 @@ def test_ogr_gpkg_36():
pytest.fail()
f = None

# Just change the name, and run it outside an existing transaction
lyr.StartTransaction()
new_field_defn = ogr.FieldDefn('baw2', ogr.OFTString)
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0
lyr.CommitTransaction()

# Just change the name, and run it under an existing transaction
lyr.StartTransaction()
new_field_defn = ogr.FieldDefn('baw', ogr.OFTString)
assert lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG) == 0
Expand Down Expand Up @@ -2902,7 +2939,9 @@ def test_ogr_gpkg_36():
# Unlink before AlterFieldDefn
gdal.Unlink(dbname)
with gdaltest.error_handler():
ret = lyr.AlterFieldDefn(0, ogr.FieldDefn('bar'), ogr.ALTER_ALL_FLAG)
new_field_defn = ogr.FieldDefn('bar')
new_field_defn.SetNullable(False)
ret = lyr.AlterFieldDefn(0, new_field_defn, ogr.ALTER_ALL_FLAG)
assert ret != 0
with gdaltest.error_handler():
ds = None
Expand Down
18 changes: 13 additions & 5 deletions autotest/ogr/ogr_rfc35_sqlite.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def CheckFeatures(lyr, field1='foo5', field2='bar10', field3='baz15', field4='ba
feat = lyr.GetNextFeature()
i = i + 1



def CheckColumnOrder(lyr, expected_order):

Expand Down Expand Up @@ -224,19 +224,27 @@ def test_ogr_rfc35_sqlite_3():

CheckFeatures(lyr, field3='baz25')

fd = ogr.FieldDefn("baz5", ogr.OFTString)
fd = ogr.FieldDefn("baz5_tmp", ogr.OFTString)
fd.SetWidth(5)

lyr_defn = lyr.GetLayerDefn()
lyr.AlterFieldDefn(lyr_defn.GetFieldIndex("baz25"), fd, ogr.ALTER_ALL_FLAG)

CheckFeatures(lyr, field3='baz5')
CheckFeatures(lyr, field3='baz5_tmp')

lyr_defn = lyr.GetLayerDefn()
fld_defn = lyr_defn.GetFieldDefn(lyr_defn.GetFieldIndex('baz5'))
fld_defn = lyr_defn.GetFieldDefn(lyr_defn.GetFieldIndex('baz5_tmp'))
assert fld_defn.GetWidth() == 5

CheckFeatures(lyr, field3='baz5')
CheckFeatures(lyr, field3='baz5_tmp')

# Change only name
fd = ogr.FieldDefn("baz5", ogr.OFTString)
fd.SetWidth(5)
lyr.AlterFieldDefn(lyr_defn.GetFieldIndex("baz5_tmp"), fd, ogr.ALTER_ALL_FLAG)

fld_defn = lyr_defn.GetFieldDefn(lyr_defn.GetFieldIndex('baz5'))
assert fld_defn.GetWidth() == 5

###############################################################################
# Test AlterFieldDefn() for change of type
Expand Down
1 change: 0 additions & 1 deletion ogr/ogrsf_frmts/gpkg/ogr_geopackage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ class GDALGeoPackageDataset final : public OGRSQLiteBaseDataSource, public GDALG

private:

OGRErr PragmaCheck(const char * pszPragma, const char * pszExpected, int nRowsExpected);
OGRErr SetApplicationAndUserVersionId();
bool ReOpenDB();
bool OpenOrCreateDB( int flags );
Expand Down
109 changes: 56 additions & 53 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,56 +271,6 @@ bool GDALGeoPackageDataset::ReOpenDB()
return OpenOrCreateDB(SQLITE_OPEN_READWRITE);
}

/* Returns the first row of first column of SQL as integer */
OGRErr GDALGeoPackageDataset::PragmaCheck(
const char * pszPragma, const char * pszExpected, int nRowsExpected )
{
CPLAssert( pszPragma != nullptr );
CPLAssert( pszExpected != nullptr );
CPLAssert( nRowsExpected >= 0 );

char **papszResult = nullptr;
int nRowCount = 0;
int nColCount = 0;
char *pszErrMsg = nullptr;

int rc = sqlite3_get_table(
hDB,
CPLSPrintf("PRAGMA %s", pszPragma),
&papszResult, &nRowCount, &nColCount, &pszErrMsg );

if( rc != SQLITE_OK )
{
CPLError( CE_Failure, CPLE_AppDefined,
"Unable to execute PRAGMA %s: %s", pszPragma,
pszErrMsg ? pszErrMsg : "(null)" );
sqlite3_free( pszErrMsg );
return OGRERR_FAILURE;
}

if ( nRowCount != nRowsExpected )
{
CPLError( CE_Failure, CPLE_AppDefined,
"bad result for PRAGMA %s, got %d rows, expected %d",
pszPragma, nRowCount, nRowsExpected );
sqlite3_free_table(papszResult);
return OGRERR_FAILURE;
}

if ( nRowCount > 0 && ! EQUAL(papszResult[1], pszExpected) )
{
CPLError( CE_Failure, CPLE_AppDefined,
"invalid %s (expected '%s', got '%s')",
pszPragma, pszExpected, papszResult[1]);
sqlite3_free_table(papszResult);
return OGRERR_FAILURE;
}

sqlite3_free_table(papszResult);

return OGRERR_NONE;
}

static OGRErr GDALGPKGImportFromEPSG(OGRSpatialReference *poSpatialRef,
int nEPSGCode)
{
Expand Down Expand Up @@ -5959,7 +5909,13 @@ OGRLayer * GDALGeoPackageDataset::ExecuteSQL( const char *pszSQLCommand,
}

/* -------------------------------------------------------------------- */
/* Intercept ALTER TABLE ... RENAME TO */
/* Intercept ALTER TABLE src_table RENAME TO dst_table */
/* and ALTER TABLE table RENAME COLUMN src_name TO dst_name */
/* and ALTER TABLE table DROP COLUMN col_name */
/* */
/* We do this because SQLite mechanisms can't deal with updating */
/* literal values in gpkg_ tables that refer to table and column */
/* names. */
/* -------------------------------------------------------------------- */
if( STARTS_WITH_CI(osSQLCommand, "ALTER TABLE ") )
{
Expand All @@ -5971,15 +5927,62 @@ OGRLayer * GDALGeoPackageDataset::ExecuteSQL( const char *pszSQLCommand,
const char* pszSrcTableName = papszTokens[2];
const char* pszDstTableName = papszTokens[5];
OGRGeoPackageTableLayer* poSrcLayer =
(OGRGeoPackageTableLayer*)GetLayerByName(
SQLUnescape(pszSrcTableName));
dynamic_cast<OGRGeoPackageTableLayer*>(GetLayerByName(
SQLUnescape(pszSrcTableName)));
if( poSrcLayer )
{
poSrcLayer->RenameTo( SQLUnescape(pszDstTableName) );
CSLDestroy(papszTokens);
return nullptr;
}
}
/* ALTER TABLE table RENAME COLUMN src_name TO dst_name */
else if( CSLCount(papszTokens) == 8 &&
EQUAL(papszTokens[3], "RENAME") &&
EQUAL(papszTokens[4], "COLUMN") &&
EQUAL(papszTokens[6], "TO") )
{
const char* pszTableName = papszTokens[2];
const char* pszSrcColumn = papszTokens[5];
const char* pszDstColumn = papszTokens[7];
OGRGeoPackageTableLayer* poLayer =
dynamic_cast<OGRGeoPackageTableLayer*>(GetLayerByName(
SQLUnescape(pszTableName)));
if( poLayer )
{
int nSrcFieldIdx = poLayer->GetLayerDefn()->GetFieldIndex(SQLUnescape(pszSrcColumn));
if( nSrcFieldIdx >= 0 )
{
// OFTString or any type will do as we just alter the name
// so it will be ignored.
OGRFieldDefn oFieldDefn(SQLUnescape(pszDstColumn), OFTString);
poLayer->AlterFieldDefn(nSrcFieldIdx, &oFieldDefn, ALTER_NAME_FLAG);
CSLDestroy(papszTokens);
return nullptr;
}
}
}
/* ALTER TABLE table DROP COLUMN col_name */
else if( CSLCount(papszTokens) == 6 &&
EQUAL(papszTokens[3], "DROP") &&
EQUAL(papszTokens[4], "COLUMN") )
{
const char* pszTableName = papszTokens[2];
const char* pszColumName = papszTokens[5];
OGRGeoPackageTableLayer* poLayer =
dynamic_cast<OGRGeoPackageTableLayer*>(GetLayerByName(
SQLUnescape(pszTableName)));
if( poLayer )
{
int nFieldIdx = poLayer->GetLayerDefn()->GetFieldIndex(SQLUnescape(pszColumName));
if( nFieldIdx >= 0 )
{
poLayer->DeleteField(nFieldIdx);
CSLDestroy(papszTokens);
return nullptr;
}
}
}
CSLDestroy(papszTokens);
}

Expand Down
Loading

0 comments on commit 4f2cb6f

Please sign in to comment.