[GRASS-dev] string madness

[Glynn Clements]

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 at gclements.plus.com>