[GRASS-dev] [GRASS GIS] #391: SQLite driver doesn't complain enough with stupid mistakes

GDAL · December 6, 2008, 2:18am

#391: SQLite driver doesn't complain enough with stupid mistakes
-------------------------+--------------------------------------------------
Reporter: hamish | Owner: grass-dev@lists.osgeo.org
Type: defect | Status: new
Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Keywords: sqlite | Platform: Linux
Cpu: Unspecified |
-------------------------+--------------------------------------------------
Using the SQLite driver, the following v.db.addcol command runs without
error:

{{{
G64:spearf> g.copy vect=archsites,tmp_arch
G64:spearf> v.db.addcol tmp_arch column='x double y double'
}}}

(typo, no comma between column defs)

but when you try and do something with it you get an error:

{{{
G64:spearf> v.info -c tmp_arch
Displaying column types/names for database connection of layer 1:
WARNING: SQLite driver: unable to parse decltype: double y double
WARNING: SQLite driver: unable to parse decltype: double y double
WARNING: SQLite driver: column 'x', SQLite type 2 is not supported
INTEGER|cat
CHARACTER|str1
}}}

a fatal error from v.db.addcol or from the driver would be nice.

Hamish

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/391>
GRASS GIS <http://grass.osgeo.org>

neteler · December 8, 2008, 7:19pm

GRASS GIS wrote:

G64:spearf> g.copy vect=archsites,tmp_arch
G64:spearf> v.db.addcol tmp_arch column='x double y double'

"columns" is a STRING, so in theory there are (other) cases that more than
two parameters can be listed, e.g. the SQL statements.

Question: could we add in the parser something like

opt->pairanswer = YES|NO,

and enforce pairs in case of YES? This would be useful as well
for east_north coordinates and such. Default NULL/NO for compatibility.

?

Dunno if we can change the struct (yet another API change?),
Markus

Hamish · December 9, 2008, 6:34am

Hamish wrote:
> G64:spearf> g.copy vect=archsites,tmp_arch
> G64:spearf> v.db.addcol tmp_arch column='x double y double'

[missing comma in columns='' didn't cause an error & left it broken]

Markus Neteler wrote:

"columns" is a STRING, so in theory there are (other) cases that
more than two parameters can be listed, e.g. the SQL statements.

I meant could the SQLite driver to throw an error, not G_parser().

Question: could we add in the parser something like
opt->pairanswer = YES|NO,

and enforce pairs in case of YES? This would be useful as
well for east_north coordinates and such. Default NULL/NO for
compatibility.
?

Dunno if we can change the struct (yet another API change?),

AFAIK that is already there & working in the parser so nothing new needed.
If key_desc has a comma in it, the parser looks for a pair(|set),
e.g. opt->key_desc = "x,y";

G64> d.barscale at=50 --q
ERROR: option <at> must be provided in multiples of 2
You provided 1 items:
50

G64> d.barscale at=50, --q
ERROR: option <at> must be provided in multiples of 2
You provided 1 items:
50,

G64> d.barscale at=50,50,50 --q
ERROR: option <at> must be provided in multiples of 2
You provided 3 items:
50,50,50

G64> d.legend elevation.dem at=80,10,5 --q
ERROR: option <at> must be provided in multiples of 4
You provided 3 items:
80,10,5

G64> d.legend elevation.dem at=80,10,5,10,11 --q
ERROR: option <at> must be provided in multiples of 4
You provided 5 items:
80,10,5,10,11

see check_multiple_opts() in lib/gis/parser.c

Hamish

Glynn_Clements1 · December 9, 2008, 7:05am

Markus Neteler wrote:

> G64:spearf> g.copy vect=archsites,tmp_arch
> G64:spearf> v.db.addcol tmp_arch column='x double y double'
"columns" is a STRING, so in theory there are (other) cases that more than
two parameters can be listed, e.g. the SQL statements.

Question: could we add in the parser something like

opt->pairanswer = YES|NO,

and enforce pairs in case of YES? This would be useful as well
for east_north coordinates and such. Default NULL/NO for compatibility.

That is already provided by opt->key_desc. If the key_desc field
contains N commas, answers must be provided in multiples of N+1. This
is commonly used for coordinate pairs.

However, database columns are specified with one answer per column,
containing both name and type separated by spaces.

It would be possible to change the column= option to use key_desc,
requiring it to be entered as e.g.

column=x,integer,y,'double precision',z,'varchar(50)'.

But I'm not sure whether that helps; I suspect that users might forget
quotes when a type contains spaces or parentheses, whereas the current
syntax means that they're always required.

The main limitation with key_desc is that all answers have to have the
same "type" (opt->type, opt->gisprompt, opt->options, etc). This means
that you can't e.g. combine r.series output= and method= options:

r.series output=out.mean,out.median,... method=mean,median,...

into a single option like:

r.series output=out.mean,mean,out.median,median,...

without losing the type-specific input mechanisms in the GUI (the type
would have to be just "string").

--
Glynn Clements <glynn@gclements.plus.com>

GDAL · December 9, 2008, 10:55am

#391: SQLite driver doesn't complain enough with stupid mistakes
-----------------------+----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Resolution: | Keywords: sqlite
  Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by neteler):

If desired in the SQLite driver, only SQL parsing might solve the problem
(complicated).

For now, in r34809 I have added a check to v.db.addcol to reject odd
column definitions comparing number of tokens against number of desired
columns to be created.

Someone needs to forward-port to trunk/Python this test if desired.

Markus

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/391#comment:1>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 9, 2008, 11:53am

#391: SQLite driver doesn't complain enough with stupid mistakes
-----------------------+----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Resolution: | Keywords: sqlite
  Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by glynn):

Replying to [comment:1 neteler]:
> If desired in the SQLite driver, only SQL parsing might solve the
problem (complicated).
>
> For now, in r34809 I have added a check to v.db.addcol to reject odd
column definitions comparing number of tokens against number of desired
columns to be created.

So presumably it won't accept column types which contain spaces ("double
precision", "character varying", "varchar (50)", etc)?

> Someone needs to forward-port to trunk/Python this test if desired.

Not desired.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/391#comment:2>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 9, 2008, 12:06pm

#391: SQLite driver doesn't complain enough with stupid mistakes
-----------------------+----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Resolution: | Keywords: sqlite
  Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by glynn):

Replying to [comment:1 neteler]:

> If desired in the SQLite driver, only SQL parsing might solve the
problem (complicated).

The main problem is that, although DBMI specifies an "add column" method,
none of the drivers implement it. So v.db.addcol just executes an SQL
"ALTER TABLE ..." statement.

So the only way to trap this in the driver would be to parse every SQL
statement which is passed to it, which I don't expect to happen.

BTW, even if all of the drivers were updated to implement the add_column
method, and a corresponding db.addcol module was created, you could still
get exactly the same problem from people/code adding columns or creating
tables manually via e.g. db.execute or the sqlite3 command.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/391#comment:3>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 9, 2008, 12:45pm

#391: SQLite driver doesn't complain enough with stupid mistakes
-----------------------+----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Resolution: | Keywords: sqlite
  Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Comment (by neteler):

Replying to [comment:2 glynn]:
> Replying to [comment:1 neteler]:
> > If desired in the SQLite driver, only SQL parsing might solve the
problem (complicated).
> >
> > For now, in r34809 I have added a check to v.db.addcol to reject odd
column definitions comparing number of tokens against number of desired
columns to be created.
>
> So presumably it won't accept column types which contain spaces ("double
precision", "character varying", "varchar (50)", etc)?

Right. r34809 reverted.

Markus

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/391#comment:4>
GRASS GIS <http://grass.osgeo.org>

GDAL · August 19, 2012, 10:40pm

#391: SQLite driver doesn't complain enough with stupid mistakes
-----------------------+----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: major | Milestone: 6.4.0
Component: Database | Version: svn-develbranch6
Resolution: fixed | Keywords: sqlite
  Platform: Linux | Cpu: Unspecified
-----------------------+----------------------------------------------------
Changes (by neteler):

* status: new => closed
* resolution: => fixed

Comment:

The intentionally broken command (missing comma) from the original
ticket is no longer accepted:

{{{
GRASS 6.4.3svn (spearfish60):~ > v.db.addcol tmp_arch column='x double y
double'
DBMI-SQLite driver error:
Error in sqlite3_prepare():
duplicate column name: x

ERROR: Error while executing: 'ALTER TABLE tmp_arch ADD COLUMN x double y
double
'
ERROR: Cannot continue (problem adding column).
}}}

Hence the ticket can be closed.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/391#comment:5>
GRASS GIS <http://grass.osgeo.org>