[GRASS-dev] string madness

[Markus_Neteler1]

Hi,

I am trying to fetch a "date" column in the sqlite driver but
don't quite arrive (just fixed an apparent bug in the column
detection there, but...).
Below is the query, the "datacens" is of special interest.
The points map was imported from PostgreSQL with v.in.ogr
using now the SQLite driver. Checking the table with "sqlite3"
doesn't show any problems, the attribute table looks ok.

But db.select omits to show the "datacens" column contents (ORDER BY
works, not shown here to keep it short):

echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select
cat|x_gid|adulti|altitudine|bab_fixed|babesia|bor_fixed|borrelia|cat_|classe|codice|comune|condatm|datacens|datains|datamod|ehr_fixed|ehrlichia|est|fauna|gid|larve|latitude|localita|longitude|ninfe|nord|nsito|nzecche|ric_fixed|rickettsia
D0/0: col:
D0/0: col: f
D0/0: col: n
D0/0: col: n
D0/0: col: Zoldo Alto
D0/0: col: soleggiato
D0/0: col: 2001-04-03 <--- OK!
D0/0: col:
D0/0: col:
D0/0: col: n
D0/0: col: n
D0/0: col: cervo, capriolo
D0/0: col: Le Vare
D0/0: col: 24p
D0/0: col:
D0/0: col: t
2|86|0|1250||f|n|n|114|24|9|Zoldo Alto|soleggiato|0|||n|n|2296670|cervo, capriolo|85|0|46|Le Vare|12|0|5136625|24p|0||t

In the output line the date entry is missing.

Above output was generated with this modification:

db/drivers/sqlite/fetch.c
...
        switch ( litetype ) {
            case SQLITE_TEXT:
                G_debug(0, "col: %s", sqlite3_column_text ( c->statement, col));
                db_set_string ( &(value->s),
                                sqlite3_column_text ( c->statement, col) );
                break;
...

Apparently the db_set_string() fails.
Adding debug output to db_set_string() in lib/db/dbmi_base/string.c
shows that the date string does not "reach" db_set_string() above.
Maybe the '-' chars in 2001-04-03 are evil for db_set_string()?

I have spent meanwhile hours on this and don't know how to fix it (at
least SQLite date column order now works!).
Help appreciated,

Markus

[Daniel_Calvelo]

Hi Markus,

On 3/17/07, Markus Neteler <neteler@itc.it> wrote:
[...]

Apparently the db_set_string() fails.
Adding debug output to db_set_string() in lib/db/dbmi_base/string.c
shows that the date string does not "reach" db_set_string() above.
Maybe the '-' chars in 2001-04-03 are evil for db_set_string()?

What reaches db_set_string()? NULL?

Don't you get a warning when compiling? sqlite3_column_text() returns
a const unsigned char * and db_set_string expects a plain char *. My
guess is casting problems.

Daniel.

--
-- Daniel Calvelo Aros

[Markus_Neteler1]

Hi Daniel,

On Sat, Mar 17, 2007 at 11:10:27AM -0500, Daniel Calvelo wrote:

Hi Markus,

On 3/17/07, Markus Neteler <neteler@itc.it> wrote:
[...]
>Apparently the db_set_string() fails.
>Adding debug output to db_set_string() in lib/db/dbmi_base/string.c
>shows that the date string does not "reach" db_set_string() above.
>Maybe the '-' chars in 2001-04-03 are evil for db_set_string()?

What reaches db_set_string()? NULL?

err, how to find out?
With 'ddd' I never reach any DBMI code, it get's me into
XDR and then I receive the result. "step" doesn't step into
DBMI.

Don't you get a warning when compiling? sqlite3_column_text() returns
a const unsigned char * and db_set_string expects a plain char *.i

Indeed:
fetch.c:121: warning: passing argument 2 of ‘db_set_string’ discards qualifiers from pointer target type

My guess is casting problems.

That might be the case (no idea how to declare it, though).

Markus

[Glynn_Clements1]

Daniel Calvelo wrote:

Don't you get a warning when compiling? sqlite3_column_text() returns
a const unsigned char * and db_set_string expects a plain char *. My
guess is casting problems.

On all common platforms, casts between various permutations of
const and unsigned char pointers are harmless.

In order for this to matter, the compiler would have to store
const/mutable or signed/unsigned chars differently, which isn't the
case for any platform on which GRASS is likely to run.

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

[Glynn_Clements1]

Markus Neteler wrote:

> >Apparently the db_set_string() fails.
> >Adding debug output to db_set_string() in lib/db/dbmi_base/string.c
> >shows that the date string does not "reach" db_set_string() above.
> >Maybe the '-' chars in 2001-04-03 are evil for db_set_string()?
>
> What reaches db_set_string()? NULL?

err, how to find out?
With 'ddd' I never reach any DBMI code, it get's me into
XDR and then I receive the result. "step" doesn't step into
DBMI.

The DBMI drivers run as separate processes. You would need to first
debug the client (db.select) up to the point where it starts the
driver (db_start_driver), then start debugging driver/db/sqlite,
attaching to the child process. Once you've attached, you need to
allow the client to continue running.

But in this case, I think that the problem is that the common
dbmi_driver code is reading value->t (because the column has type
datetime) and sending that to the client, but the SQLite
implementation of db__driver_fetch (db/drivers/sqlite/fetch.c) is
filling in value->s (because the data is stored as SQLITE_TEXT).

To fix this, the SQLite driver would need to check the column type
reported to the client (sqltype) rather than the type used within
SQLite (litetype), and parse the textual form into the value->t field
(of type dbDateTime).

I can't really look into this much further, as I don't have SQLite
installed here.

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

[Markus_Neteler1]

On Sat, Mar 17, 2007 at 07:37:48PM +0000, Glynn Clements wrote:

Markus Neteler wrote:

> > >Apparently the db_set_string() fails.
> > >Adding debug output to db_set_string() in lib/db/dbmi_base/string.c
> > >shows that the date string does not "reach" db_set_string() above.
> > >Maybe the '-' chars in 2001-04-03 are evil for db_set_string()?
> >
> > What reaches db_set_string()? NULL?
>
> err, how to find out?
> With 'ddd' I never reach any DBMI code, it get's me into
> XDR and then I receive the result. "step" doesn't step into
> DBMI.

The DBMI drivers run as separate processes. You would need to first
debug the client (db.select) up to the point where it starts the
driver (db_start_driver), then start debugging driver/db/sqlite,
attaching to the child process. Once you've attached, you need to
allow the client to continue running.

I debugged db.select but then bailed out when reaching XDR.
I am not sure how to attach to a child process (I am using 'ddd').
Maybe some notes could be added to
http://grass.gdf-hannover.de/wiki/GRASS_Debugging
(to avoid future questions) - or I can add notes once I know
how to do it.

But in this case, I think that the problem is that the common
dbmi_driver code is reading value->t (because the column has type
datetime) and sending that to the client, but the SQLite
implementation of db__driver_fetch (db/drivers/sqlite/fetch.c) is
filling in value->s (because the data is stored as SQLITE_TEXT).

Ah, this detail I was missing. Looking at the Postgresql driver, I
see that now - didn't remember that value->t was there. I was sort
of blind when looking at include/dbmi.h.

To fix this, the SQLite driver would need to check the column type

That's easy, in my local version it was already there.
Now in CVS.

reported to the client (sqltype) rather than the type used within
SQLite (litetype), and parse the textual form into the value->t field
(of type dbDateTime).

I have added this in CVS now, inspired by the Postgresql driver.
It still doesn't work, sigh. Still no date output in db.select.

I can't really look into this much further, as I don't have SQLite
installed here.

Sure, no problem.
It would be very helpful to have a function to simply print
the current content of the value structure. Maybe it is just
to make a function to print more or less the results of:

grep return lib/db/dbmi_base/value.c | grep value
?

Anyway, I assume that value->t is populated now and that the
content gets lost later.

Markus

[Glynn_Clements1]

Markus Neteler wrote:

I debugged db.select but then bailed out when reaching XDR.
I am not sure how to attach to a child process (I am using 'ddd').
Maybe some notes could be added to
http://grass.gdf-hannover.de/wiki/GRASS_Debugging
(to avoid future questions) - or I can add notes once I know
how to do it.

I haven't used DDD in years. In gdb, you use "attach <pid>" to attach
to an existing process.

> To fix this, the SQLite driver would need to check the column type

That's easy, in my local version it was already there.
Now in CVS.

> reported to the client (sqltype) rather than the type used within
> SQLite (litetype), and parse the textual form into the value->t field
> (of type dbDateTime).

I have added this in CVS now, inspired by the Postgresql driver.
It still doesn't work, sigh. Still no date output in db.select.

  switch ( litetype ) {
      case SQLITE_TEXT:
    if (sqltype == 6 ) { /* date string */

I don't know where that 6 comes from; it should probably be
DB_SQL_TYPE_DATE (which is 9).

However, having looked at this some more, I don't see how you can have
a "date" column in an SQLite database. The SQLite driver's version of
db__driver_create_table() just creates a "text" column, with no
indication that the column actually contains dates. Unless *something*
is storing the fact that the column contains dates, the value should
simply be treated as text.

What does db.describe have to say about the table?

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

[Markus_Neteler1]

On Sat, Mar 17, 2007 at 09:15:46PM +0000, Glynn Clements wrote:

Markus Neteler wrote:

> I debugged db.select but then bailed out when reaching XDR.
> I am not sure how to attach to a child process (I am using 'ddd').
> Maybe some notes could be added to
> http://grass.gdf-hannover.de/wiki/GRASS_Debugging
> (to avoid future questions) - or I can add notes once I know
> how to do it.

I haven't used DDD in years.

[ I am just a poor geographer ]

In gdb, you use "attach <pid>" to attach
to an existing process.

OK, AFAIK I can enter gdb commands in 'ddd', too.

> > To fix this, the SQLite driver would need to check the column type
>
> That's easy, in my local version it was already there.
> Now in CVS.
>
> > reported to the client (sqltype) rather than the type used within
> > SQLite (litetype), and parse the textual form into the value->t field
> > (of type dbDateTime).
>
> I have added this in CVS now, inspired by the Postgresql driver.
> It still doesn't work, sigh. Still no date output in db.select.

  switch ( litetype ) {
      case SQLITE_TEXT:
    if (sqltype == 6 ) { /* date string */

I don't know where that 6 comes from; it should probably be
DB_SQL_TYPE_DATE (which is 9).

I have no clue. Here debug output (the map was imported with v.in.ogr
from Postgresql where the column is of "date" type):

g.gisenv set=DEBUG=3
echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select
...
D3/3: name = '$GISDBASE/$LOCATION_NAME/$MAPSET/sqlite.db'
D3/3: tokens[0] = $GISDBASE
D3/3: -> /ssi0/ssi/neteler/grassdata
D3/3: tokens[1] = $LOCATION_NAME
D3/3: -> belluno_GBovest
D3/3: tokens[2] = $MAPSET
D3/3: -> zecche_sqlite
D3/3: tokens[3] = sqlite.db
D2/3: name2 = '/ssi0/ssi/neteler/grassdata/belluno_GBovest/zecche_sqlite/sqlite.db'
D3/3: Escaped SQL: SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens

D3/3: describe_table()
D3/3: ncols = 34
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 2
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 1
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: nkcols = 34
D3/3: litetype = 1
D2/3: col: cat, nkcols 0, litetype : 1, sqltype 3
D3/3: litetype = 1
D2/3: col: x_gid, nkcols 1, litetype : 1, sqltype 3
D3/3: litetype = 1
D2/3: col: adulti, nkcols 2, litetype : 1, sqltype 3
D3/3: litetype = 1
D2/3: col: altitudine, nkcols 3, litetype : 1, sqltype 3
D3/3: litetype = 3
D2/3: col: bab_fixed, nkcols 4, litetype : 3, sqltype 13
D3/3: litetype = 3
D2/3: col: babesia, nkcols 5, litetype : 3, sqltype 13
D3/3: litetype = 3
D2/3: col: bor_fixed, nkcols 6, litetype : 3, sqltype 13
D3/3: litetype = 3
D2/3: col: borrelia, nkcols 7, litetype : 3, sqltype 13
D3/3: litetype = 1
D2/3: col: cat_, nkcols 8, litetype : 1, sqltype 3
D3/3: litetype = 1
D2/3: col: classe, nkcols 9, litetype : 1, sqltype 3
D3/3: litetype = 1
D2/3: col: codice, nkcols 10, litetype : 1, sqltype 3
D3/3: litetype = 3
D2/3: col: comune, nkcols 11, litetype : 3, sqltype 13
D3/3: litetype = 3
D2/3: col: condatm, nkcols 12, litetype : 3, sqltype 13
D3/3: litetype = 2
D2/3: col: datacens, nkcols 13, litetype : 2, sqltype 6
D3/3: litetype = 3
D2/3: col: datains, nkcols 14, litetype : 3, sqltype 13
D3/3: litetype = 3
...

"datacens" is the column in question.

However, having looked at this some more, I don't see how you can have
a "date" column in an SQLite database.

I cannot. It is a string column.

The SQLite driver's version of
db__driver_create_table() just creates a "text" column, with no
indication that the column actually contains dates. Unless *something*
is storing the fact that the column contains dates, the value should
simply be treated as text.

Right. That's why I am confused - the sscanf parsing should do the job.

What does db.describe have to say about the table?

db.describe zecche_BL2001_gis
...
column:datacens
description:
type:DOUBLE PRECISION
len:99999
scale:0
precision:0
default:
nullok:yes
select:?
update:?

I assume that it is now 'DOUBLE PRECISION' since this should be the
fallback if type detection fails (in GRASS!?).

If I open the table in sqlite3, it works, so the table seems to
be fine:

sqlite3 sqlite.db
SQLite version 3.3.5
Enter ".help" for instructions
sqlite> .schema
CREATE TABLE zecche_BL2001_gis (cat integer, x_gid integer, adulti integer, altitudine integer, bab_fixed varchar ( 1 ), babesia varchar ( 80 ), bor_fixed varchar ( 1 ), borrelia varchar ( 80 ), cat_ integer, classe integer, codice integer, comune varchar ( 80 ), condatm varchar ( 80 ), datacens date, datains varchar ( 80 ), datamod varchar ( 80 ), ehr_fixed varchar ( 1 ), ehrlichia varchar ( 80 ), est integer, fauna varchar ( 80 ), gid integer, larve integer, latitude integer, localita varchar ( 80 ), longitude integer, ninfe integer, nord integer, nsito varchar ( 80 ), nzecche integer, ric_fixed varchar ( 1 ), rickettsia varchar ( 80 ), rilevatore varchar ( 80 ), utente varchar ( 80 ), vegetazion varchar ( 80 ));
CREATE UNIQUE INDEX zecche_BL2001_gis_cat on zecche_BL2001_gis ( cat );
sqlite> select typeof(datacens) from zecche_BL2001_gis;
text
text
text
...
sqlite> select datacens from zecche_BL2001_gis;
2001-03-28
2001-04-03
2001-08-20
...

Markus

[Glynn_Clements1]

Markus Neteler wrote:

g.gisenv set=DEBUG=3
echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select

D2/3: col: datacens, nkcols 13, litetype : 2, sqltype 6

"datacens" is the column in question.

  http://www.sqlite.org/cvstrac/fileview?f=sqlite/src/sqlite.h.in&v=1.198
says:
  #define SQLITE_INTEGER 1
  #define SQLITE_FLOAT 2
  /* #define SQLITE_TEXT 3 // See below */
  #define SQLITE_BLOB 4
  #define SQLITE_NULL 5

IOW, litetype == SQLITE_FLOAT, sqltype == DB_SQL_TYPE_DOUBLE_PRECISION.

If DBMI thinks that the column contains a float, trying to parse
YYYY-MM-DD as a float is going to fail.

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

[Markus_Neteler1]

On Sat, Mar 17, 2007 at 09:59:12PM +0000, Glynn Clements wrote:

Markus Neteler wrote:

> g.gisenv set=DEBUG=3
> echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select

> D2/3: col: datacens, nkcols 13, litetype : 2, sqltype 6

> "datacens" is the column in question.

  http://www.sqlite.org/cvstrac/fileview?f=sqlite/src/sqlite.h.in&v=1.198
says:
  #define SQLITE_INTEGER 1
  #define SQLITE_FLOAT 2
  /* #define SQLITE_TEXT 3 // See below */
  #define SQLITE_BLOB 4
  #define SQLITE_NULL 5

A few lines down it says:

  /*
  ** SQLite version 2 defines SQLITE_TEXT differently. To allow both
  ** version 2 and version 3 to be included, undefine them both if a
  ** conflict is seen. Define SQLITE3_TEXT to be the version 3 value.
  */
  #ifdef SQLITE_TEXT
  # undef SQLITE_TEXT
  #else
  # define SQLITE_TEXT 3
  #endif
  #define SQLITE3_TEXT 3

A SQLITE3_TEXT seems to be there. (?)

IOW, litetype == SQLITE_FLOAT, sqltype == DB_SQL_TYPE_DOUBLE_PRECISION.

Yes, the table description is going wrong in GRASS.

If DBMI thinks that the column contains a float, trying to parse
YYYY-MM-DD as a float is going to fail.

Right. Some analysis:

echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select
...
D3/3: litetype = 3
D3/3: litetype = 3
D3/3: litetype = 2 <--- ZAP
D3/3: litetype = 3
...
D3/3: litetype = 3
D2/3: col: condatm, nkcols 12, litetype : 3, sqltype 13
D3/3: litetype = 2 <--- ZAP
D2/3: col: datacens, nkcols 13, litetype : 2, sqltype 6
D3/3: litetype = 3
D2/3: col: datains, nkcols 14, litetype : 3, sqltype 13
...
D3/3: col 10, litetype 1, sqltype 3: val = '87'
D3/3: col 11, litetype 3, sqltype 13: val = 'Zoldo Alto'
D3/3: col 12, litetype 3, sqltype 13: val = 'soleggiato'
D3/3: col 13, litetype 3, sqltype 6: val = '2001-08-01' <-- BETTER
D3/3: sqlite fetched date: 2001-08-01
D3/3: col 14, litetype 3, sqltype 13: val = ''

describe.c: db_set_column_host_type(column, litetype);

in fetch.c, I had changed:
        litetype = db_get_column_host_type(column);
to
        litetype = sqlite3_column_type ( c->statement, col );
to obtain
        D3/3: col 13, litetype 3, sqltype 6: val = '2001-08-01'
which makes the SWITCH statement in fetch.c work.

But in describe.c the function
        db_set_column_host_type(column, litetype);
is still used which reports litetype 2. Not sure how to fix this one.

With DEBUG=4 I get output from get_column_info() in describe.c

...
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
D2/4: col: condatm, nkcols 12, litetype : 3, sqltype 13
D4/4: decltype = date <--- from PostgreSQL originally
D3/4: litetype = 2 <--- should be 3 ideally
D2/4: col: datacens, nkcols 13, litetype : 2, sqltype 6 <-- the problem
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
D2/4: col: datains, nkcols 14, litetype : 3, sqltype 13
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
...

#### some minutes later ###########

I GOT IT!

this patch:
diff -u -r1.4 describe.c
--- describe.c 4 Jul 2006 09:24:14 -0000 1.4
+++ describe.c 17 Mar 2007 22:34:08 -0000
@@ -199,7 +199,11 @@
        *litetype = sqlite3_column_type ( statement, col );
     }

- G_debug ( 3, "litetype = %d", *litetype );
+ /* Date support hack */
+ if ( strcmp(decltype, "date") == 0 ) {
+ *litetype = SQLITE_TEXT;
+ G_debug ( 4, " date found, new litetype = %d", *litetype );
+ }

     switch ( *litetype) {
        case SQLITE_INTEGER:

does the job:

D4/4: decltype = varchar ( 80 )
D2/4: col: condatm, nkcols 12, litetype : 3, sqltype 13
D4/4: decltype = date
D4/4: date found, new litetype = 3
D2/4: col: datacens, nkcols 13, litetype : 3, sqltype 13
D4/4: decltype = varchar ( 80 )
D2/4: col: datains, nkcols 14, litetype : 3, sqltype 13

# TEST
echo "SELECT * from zecche_BL2001_gis WHERE comune='Zoldo Alto' ORDER BY datacens" | db.select
cat|x_gid|adulti|altitudine|bab_fixed|babesia|bor_fixed|borrelia|cat_|classe|codice|comune|condatm|datacens|datains|datamod|ehr_fixed|ehrlichia|est|fauna|gid|larve|latitude|localita|longitude|ninfe|nord|nsito|nzecche|ric_fixed|rickettsia|rilevatore|utente|vegetazion
2|86|0|1250||f|n|n|114|24|9|Zoldo Alto|soleggiato|2001-04-03|||n|n|2296670|cervo, capriolo|85|0|46|Le Vare|12|0|5136625|24p|0||t||ULSSBL|formazioni di conifere d'alta quota

-> 2001-04-03 is finally there...

That was tough

Thanks, Glynn, for you support and for getting me into the right direction!
Submitted to CVS.

Markus

[Glynn_Clements1]

Markus Neteler wrote:

With DEBUG=4 I get output from get_column_info() in describe.c

...
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
D2/4: col: condatm, nkcols 12, litetype : 3, sqltype 13
D4/4: decltype = date <--- from PostgreSQL originally
D3/4: litetype = 2 <--- should be 3 ideally
D2/4: col: datacens, nkcols 13, litetype : 2, sqltype 6 <-- the problem
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
D2/4: col: datains, nkcols 14, litetype : 3, sqltype 13
D4/4: decltype = varchar ( 80 )
D3/4: litetype = 3
...

#### some minutes later ###########

I GOT IT!

this patch:
diff -u -r1.4 describe.c
--- describe.c 4 Jul 2006 09:24:14 -0000 1.4
+++ describe.c 17 Mar 2007 22:34:08 -0000
@@ -199,7 +199,11 @@
        *litetype = sqlite3_column_type ( statement, col );
     }

- G_debug ( 3, "litetype = %d", *litetype );
+ /* Date support hack */
+ if ( strcmp(decltype, "date") == 0 ) {
+ *litetype = SQLITE_TEXT;
+ G_debug ( 4, " date found, new litetype = %d", *litetype );
+ }

     switch ( *litetype) {
        case SQLITE_INTEGER:

This patch is entirely inappropriate. You're adding a hack to fix a
specific broken table. Instead, you need to figure out why the table
is getting created wrong in the first place. Once that's fixed, you
don't need this hack.

Submitted to CVS.

And about to be reverted.

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

[Wolf]

On 18.03.2007 00:48, Glynn Clements wrote:

        case SQLITE_INTEGER:

This patch is entirely inappropriate. You're adding a hack to fix a
specific broken table. Instead, you need to figure out why the table
is getting created wrong in the first place. Once that's fixed, you
don't need this hack.

Well this occurs if you create an sqlite table with type date (sqlite
allows this since sqlite is typeless and it won't complain).

Then if you connect to it using v.db.connect, it will end up as a DOUBLE
PRECISION layer which is bad. IMO it should default to VARCHAR(255)

Here are the steps I did to reproduce this problem:
first create a table in sqlite:
create table foo(cat int, datum date);
quit and now connect this to a vector map

GRASS 6.3.cvs > v.db.connect layer=2 map=roads table=foo
WARNING: The table <foo> is now part of vector map <roads> and may be
         deleted or overwritten by GRASS modules
WARNING: Select privileges were granted on the table

GRASS 6.3.cvs > v.info map=roads layer=2 -c
Displaying column types/names for database connection of layer 2:
INTEGER|cat
DOUBLE PRECISION|datum
^^^^^^^^^^^^^^^^
This is not right... it should be at least string...

So in other words this might really be an issue... perhaps we should
treat everything which is not numeric as a string when dealing with SQLite?

--Wolf

--

<:3 )---- Wolf Bergenheim ----( 8:>

[Glynn_Clements1]

Wolf Bergenheim wrote:

>> case SQLITE_INTEGER:
>
> This patch is entirely inappropriate. You're adding a hack to fix a
> specific broken table. Instead, you need to figure out why the table
> is getting created wrong in the first place. Once that's fixed, you
> don't need this hack.
>

Well this occurs if you create an sqlite table with type date (sqlite
allows this since sqlite is typeless and it won't complain).

Then if you connect to it using v.db.connect, it will end up as a DOUBLE
PRECISION layer which is bad. IMO it should default to VARCHAR(255)

Here are the steps I did to reproduce this problem:
first create a table in sqlite:
create table foo(cat int, datum date);
quit and now connect this to a vector map

GRASS 6.3.cvs > v.db.connect layer=2 map=roads table=foo
WARNING: The table <foo> is now part of vector map <roads> and may be
         deleted or overwritten by GRASS modules
WARNING: Select privileges were granted on the table

GRASS 6.3.cvs > v.info map=roads layer=2 -c
Displaying column types/names for database connection of layer 2:
INTEGER|cat
DOUBLE PRECISION|datum
^^^^^^^^^^^^^^^^
This is not right... it should be at least string...

So in other words this might really be an issue... perhaps we should
treat everything which is not numeric as a string when dealing with SQLite?

Yes. SQLite's db__driver_create_table() already does this if you
create the table that way.

I think that the problem is affinity_type (in describe.c):

  int affinity_type ( const char *declared )
  {
      char *lc;
      int aff = SQLITE_FLOAT;
  
      lc = strdup ( declared );
      G_tolcase ( lc );
  
      if ( strstr(lc,"int") )
      {
          aff = SQLITE_INTEGER;
      }
      else if ( strstr(lc,"char") || strstr(lc,"clob")
                || strstr(lc,"text") )
      {
          aff = SQLITE_TEXT;
      }
      else if ( strstr(lc,"blob") )
      {
          aff = SQLITE_BLOB;
      }
    
      return aff;
  }

But shouldn't get_column_info be using sqlite3_column_type() to get
SQLite's idea of the column type rather than trying to guess based
upon the declaration type? Oh; it already does that, but only if
there's no decltype:

    decltype = sqlite3_column_decltype ( statement, col );
    if ( decltype )
    {
  G_debug ( 4, "decltype = %s", decltype );
  *litetype = affinity_type ( decltype );
    }
    else
    {
  G_debug ( 4, "this is not a table column" );
  
  /* If there are no results it gives 0 */
  *litetype = sqlite3_column_type ( statement, col );
    }

Potential fixes (in descending order of preference):

1. Discard affinity_type() and always use sqlite3_column_type() for
the litetype.

2. Change affinity_type() to default to text (but then we need a list
of decltypes which should be treated as SQLITE_FLOAT).

3. Add strstr(lc, "date") to the SQLITE_TEXT case in affinity_type().

An additional improvement would be to modify get_column_info() to take
the decltype into account when determining the sqltype, rather than
only using the litetype. That would allow dates to be returned as
dates rather than as text. But I strongly feel that the litetype
should be what sqlite3_column_type() says it is, not what it "should"
be.

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

[Markus_Neteler1]

On Sat, Mar 17, 2007 at 10:48:33PM +0000, Glynn Clements wrote:

Markus Neteler wrote:

> With DEBUG=4 I get output from get_column_info() in describe.c
>
> ...
> D4/4: decltype = varchar ( 80 )
> D3/4: litetype = 3
> D2/4: col: condatm, nkcols 12, litetype : 3, sqltype 13
> D4/4: decltype = date <--- from PostgreSQL originally
> D3/4: litetype = 2 <--- should be 3 ideally
> D2/4: col: datacens, nkcols 13, litetype : 2, sqltype 6 <-- the problem
> D4/4: decltype = varchar ( 80 )
> D3/4: litetype = 3
> D2/4: col: datains, nkcols 14, litetype : 3, sqltype 13
> D4/4: decltype = varchar ( 80 )
> D3/4: litetype = 3
> ...
>
> #### some minutes later ###########
>
> I GOT IT!
>
> this patch:
> diff -u -r1.4 describe.c
> --- describe.c 4 Jul 2006 09:24:14 -0000 1.4
> +++ describe.c 17 Mar 2007 22:34:08 -0000
> @@ -199,7 +199,11 @@
> *litetype = sqlite3_column_type ( statement, col );
> }
>
> - G_debug ( 3, "litetype = %d", *litetype );
> + /* Date support hack */
> + if ( strcmp(decltype, "date") == 0 ) {
> + *litetype = SQLITE_TEXT;
> + G_debug ( 4, " date found, new litetype = %d", *litetype );
> + }
>
> switch ( *litetype) {
> case SQLITE_INTEGER:

This patch is entirely inappropriate. You're adding a hack to fix a
specific broken table.

Accepted that my fix is broken, but the table isn't:

sqlite3 sqlite.db
sqlite> select typeof(datacens) from zecche_BL2001_gis;
text
...

sqlite> select datacens from zecche_BL2001_gis;
2001-03-28
...

It is the GRASS driver which doesn't work as needed.

Instead, you need to figure out why the table
is getting created wrong in the first place. Once that's fixed, you
don't need this hack.

The table is created correctly. Keeping the column type
helps us once the user wants to convert back from SQLite
to something else.
Note that SQLite is special about types:

sqlite3
SQLite version 3.3.6
Enter ".help" for instructions
sqlite> create table t (id bogus, data trento, created default current_date);
sqlite> insert into t (id, data) values (1, 'test');
sqlite> insert into t (id, data) values (2, 'more');
sqlite> select created from t;
2007-03-18
2007-03-18
sqlite> select * from t;
1|test|2007-03-18
2|more|2007-03-18
sqlite> select typeof(id), typeof(data), typeof(current_date) from t;
integer|text|text
integer|text|text
sqlite>

-> "bogus" and "trento" type are accepted without problems, so is "date".
It just checks what comes in and assigns the type.

I admit that I am not much expert here.

> Submitted to CVS.

And about to be reverted.

OK.

Markus

[Markus_Neteler1]

On Sat, Mar 17, 2007 at 11:58:25PM +0000, Glynn Clements wrote:

Wolf Bergenheim wrote:

...

> So in other words this might really be an issue... perhaps we should
> treat everything which is not numeric as a string when dealing with SQLite?

Yes. SQLite's db__driver_create_table() already does this if you
create the table that way.

I think that the problem is affinity_type (in describe.c):

  int affinity_type ( const char *declared )

...

But shouldn't get_column_info be using sqlite3_column_type() to get
SQLite's idea of the column type rather than trying to guess based
upon the declaration type? Oh; it already does that, but only if
there's no decltype:

This is where I got trapped and thought to fix it:

    decltype = sqlite3_column_decltype ( statement, col );
    if ( decltype )
    {
  G_debug ( 4, "decltype = %s", decltype );
  *litetype = affinity_type ( decltype );
    }
    else
    {
  G_debug ( 4, "this is not a table column" );
  
  /* If there are no results it gives 0 */
  *litetype = sqlite3_column_type ( statement, col );
    }

Potential fixes (in descending order of preference):

1. Discard affinity_type() and always use sqlite3_column_type() for
the litetype.

2. Change affinity_type() to default to text (but then we need a list
of decltypes which should be treated as SQLITE_FLOAT).

This would be probably against the philosophy of SQLite
(btw, their lists are full of people complaining about the
bad/absent "date" support).

3. Add strstr(lc, "date") to the SQLITE_TEXT case in affinity_type().

Your related patch for (3) does the job:

--- describe.c 17 Mar 2007 22:50:43 -0000 1.6
+++ describe.c 18 Mar 2007 00:01:17 -0000 1.7
@@ -251,7 +251,7 @@
         aff = SQLITE_INTEGER;
     }
     else if ( strstr(lc,"char") || strstr(lc,"clob")
- || strstr(lc,"text") )
+ || strstr(lc,"text") || strstr(lc,"date") )
     {
         aff = SQLITE_TEXT;
     }

Tested successfully with my table.

An additional improvement would be to modify get_column_info() to take
the decltype into account when determining the sqltype, rather than
only using the litetype. That would allow dates to be returned as
dates rather than as text. But I strongly feel that the litetype
should be what sqlite3_column_type() says it is, not what it "should"
be.

That might be sufficient rather than guessing around.

Markus

[Glynn_Clements1]

Markus Neteler wrote:

> This patch is entirely inappropriate. You're adding a hack to fix a
> specific broken table.

Accepted that my fix is broken, but the table isn't:

Yeah, I noticed that afterwards. I had assumed that litetype was the
actual type reported by SQLite (sqlite3_column_type). Then I
discovered that it was being synthesised by affinity_type().

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

[Glynn_Clements1]

Markus Neteler wrote:

> Potential fixes (in descending order of preference):
>
> 1. Discard affinity_type() and always use sqlite3_column_type() for
> the litetype.
>
> 2. Change affinity_type() to default to text (but then we need a list
> of decltypes which should be treated as SQLITE_FLOAT).

This would be probably against the philosophy of SQLite
(btw, their lists are full of people complaining about the
bad/absent "date" support).

> 3. Add strstr(lc, "date") to the SQLITE_TEXT case in affinity_type().

Your related patch for (3) does the job:

--- describe.c 17 Mar 2007 22:50:43 -0000 1.6
+++ describe.c 18 Mar 2007 00:01:17 -0000 1.7
@@ -251,7 +251,7 @@
         aff = SQLITE_INTEGER;
     }
     else if ( strstr(lc,"char") || strstr(lc,"clob")
- || strstr(lc,"text") )
+ || strstr(lc,"text") || strstr(lc,"date") )
     {
         aff = SQLITE_TEXT;
     }

Tested successfully with my table.

But will presumably fail if a table uses some other unknown type.

If affinity_type() is retained, it really should default to text.
AFAICT, it should always be possible to obtain a value as text, but
not all values can be converted to float. The problem is that we would
then need a list of types which should be treated as float.

> An additional improvement would be to modify get_column_info() to take
> the decltype into account when determining the sqltype, rather than
> only using the litetype. That would allow dates to be returned as
> dates rather than as text. But I strongly feel that the litetype
> should be what sqlite3_column_type() says it is, not what it "should"
> be.

That might be sufficient rather than guessing around.

Having just read the SQLite documentation, sqlite3_column_type() only
returns a valid result if the cursor is valid (i.e. if you have
performed a query and obtained results). This is consistent with
SQLite's behaviour of typing the actual values rather than the columns
which contain them.

That is presumably why affinity_type is used in place of
sqlite3_column_type().

OTOH, sqlite3_column_decltype() only returns a valid result if the
result column directly corresponds to a table column. If the result
column is an expression, it returns NULL.

That may be why affinity_type() defaults to SQLITE_FLOAT; most
expressions will probably have a numeric result. However,
affinity_type() isn't called if decltype == NULL, so this is probably
misguided.

This is all rather problematic for db__driver_describe_table(), which
has to determine the format before any data has actually been
retrieved. And all data for a column must have the same type, so
db__driver_fetch() can't look at the actual type of the data.

In which case, the best that can be achieved would be option #2 above,
but we need a list of known floating-point types. From the PostgreSQL
documentation, I get decimal, numeric, real and double precision.

Beyond that, I'm not sure that describe.c should be bothering about
SQLITE_* types at all. It seems more logical to just associate a
DB_SQL_TYPE_* type with each column, and have fetch.c reference that
directly.

In the case where decltype == NULL and sqlite3_column_type() is
actually called, I suspect that it will always return 0 (corresponding
to an sqltype of DB_SQL_TYPE_UNKNOWN) as describe_table() will
normally (always?) be called before any rows have been read.

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