[GRASS-dev] new v.in.geonames: problems with UTF-8 Unicode text

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

[...]

>>> IOW, change db/drivers/sqlite/describe.c to parse the sqlite_master
>>> table according to the syntax used by
>>> db/drivers/sqlite/create_table.c.

>> Hm. Now I see sqlite3_column_decltype () in the SQLite API
>> reference?

> Er, right. ISTR that there are reasons why the SQLite driver doesn't
> rely upon that much. However, I suspect that it will produce the same
> result as parsing the string from sqlite_master.

  Most probably.

> IIRC, the main problem is that sqlite3_column_decltype() only works
> for actual columns, not expressions, subselects etc, but the code
> in question has to be able to describe the format of rows returned
> by arbitrary SELECT statements, not just tables.

> So it falls back to sqlite3_column_type(), which returns the type
> of the column's data. but that only works if you have a valid
> row. If a select doesn't return any rows, you lose. Also,
> sqlite3_column_type() only understands null, integer, float, text
> and blob types.

  I wonder, how it's done for other RDBMS? E. g., would there be
  a table, like:

CREATE TABLE foo (foo NUMERIC, bar TEXT);

  What would be the type of the only column of the query like:

SELECT foo || bar FROM foo;

  Or, the same question for the table like:

CREATE TABLE foo (foo VARCHAR (5), bar CHAR (5));

  It seems that the only sane type to be inferred from the
  expression is TEXT (for either case.)

  And I guess, sqlite3_column_type () will return either text or
  blob.

> Apart from needing to fall-back to the data type for expressions,
> another problem is that it coerces the decltype to one of SQLite's
> limited set of types, then converts that to one of the DB_SQL_*
> types, losing information in the process.

  That doesn't feel sound.

> However, I don't know if it does this for a specific reason. I
> suspect the only way to find out is to try it and see what breaks.

[...]

[Glynn_Clements1]

Ivan Shmakov wrote:

> IIRC, the main problem is that sqlite3_column_decltype() only works
> for actual columns, not expressions, subselects etc, but the code
> in question has to be able to describe the format of rows returned
> by arbitrary SELECT statements, not just tables.

> So it falls back to sqlite3_column_type(), which returns the type
> of the column's data. but that only works if you have a valid
> row. If a select doesn't return any rows, you lose. Also,
> sqlite3_column_type() only understands null, integer, float, text
> and blob types.

  I wonder, how it's done for other RDBMS? E. g., would there be
  a table, like:

CREATE TABLE foo (foo NUMERIC, bar TEXT);

  What would be the type of the only column of the query like:

SELECT foo || bar FROM foo;

Operators such as || which operate on strings normally accept any
operand types. Anything that isn't a string will be converted to one.
But the result is always a string.

Typically, an operator is selected according to its operand types, and
the operator's result type determines the type of the result column.
E.g. suppose that you have:

  SELECT a + b FROM foo;

If a and/or b are REAL then the operator would be REAL * REAL -> REAL
and the result would be REAL. If a and b were both INT then the
operator would be INT * INT -> INT and the result would be INT.

> Apart from needing to fall-back to the data type for expressions,
> another problem is that it coerces the decltype to one of SQLite's
> limited set of types, then converts that to one of the DB_SQL_*
> types, losing information in the process.

  That doesn't feel sound.

> However, I don't know if it does this for a specific reason. I
> suspect the only way to find out is to try it and see what breaks.

One issue is that SQLite doesn't validate the column types. It uses
some heuristics to determine the affinity type, but ultimately it
allows any sequence of words along with an optional size specifier,
e.g.:

  CREATE TABLE foo ( i INT, t TEXT, b ROAST BEEF(99) );

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

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

>>> IIRC, the main problem is that sqlite3_column_decltype() only works
>>> for actual columns, not expressions, subselects etc, but the code
>>> in question has to be able to describe the format of rows returned
>>> by arbitrary SELECT statements, not just tables.

>>> So it falls back to sqlite3_column_type(), which returns the type
>>> of the column's data. but that only works if you have a valid
>>> row. If a select doesn't return any rows, you lose. Also,
>>> sqlite3_column_type() only understands null, integer, float, text
>>> and blob types.

>> I wonder, how it's done for other RDBMS?

[...]

> Operators such as || which operate on strings normally accept any
> operand types. Anything that isn't a string will be converted to one.
> But the result is always a string.

> Typically, an operator is selected according to its operand types,
> and the operator's result type determines the type of the result
> column. E.g. suppose that you have:

> SELECT a + b FROM foo;

> If a and/or b are REAL then the operator would be REAL * REAL -> REAL
> and the result would be REAL. If a and b were both INT then the
> operator would be INT * INT -> INT and the result would be INT.

  From the above I could conclude that the other RDBMS are
  generally representing the results of expressions in SELECTs as
  having one type out of a narrow set of types (in absence of
  explicit type casts.) This way, SQLite doesn't look much
  different to those.

  Or, to put it differently, I see the discussed problem as not
  being specific to SQLite.

  I suspect however, that there may be a problem with
  sqlite3_column_type () describing the value of the field of a
  given row as being of the NULL type (as it may be possible for
  the other row to have a value of a different type in the same
  field.) (And if sqlite3_column_type ()'s result could be even
  more volatile, then it is the problem.)

>>> Apart from needing to fall-back to the data type for expressions,
>>> another problem is that it coerces the decltype to one of SQLite's
>>> limited set of types, then converts that to one of the DB_SQL_*
>>> types, losing information in the process.

>> That doesn't feel sound.

>>> However, I don't know if it does this for a specific reason. I
>>> suspect the only way to find out is to try it and see what breaks.

> One issue is that SQLite doesn't validate the column types. It uses
> some heuristics to determine the affinity type, but ultimately it
> allows any sequence of words along with an optional size specifier,
> e.g.:

> CREATE TABLE foo ( i INT, t TEXT, b ROAST BEEF(99) );

  Of course. So, it becomes the user's responsibility to make
  sure that the definition of the table in question is compatible
  with the RDBMS to which it's copied.

  I guess that much the same way, once the table is defined to use
  any types specific to an RDBMS implementation, it cannot be
  readily copied to any other RDBMS.

  The only SQLite-specific problem that I see there is that it
  doesn't actually treat the type name as a constraint on the
  values put into that field. But then, ``garbage in, garbage
  out'' is the guiding principle.

[Glynn_Clements1]

Ivan Shmakov wrote:

> Operators such as || which operate on strings normally accept any
> operand types. Anything that isn't a string will be converted to one.
> But the result is always a string.

> Typically, an operator is selected according to its operand types,
> and the operator's result type determines the type of the result
> column. E.g. suppose that you have:

> SELECT a + b FROM foo;

> If a and/or b are REAL then the operator would be REAL * REAL -> REAL
> and the result would be REAL. If a and b were both INT then the
> operator would be INT * INT -> INT and the result would be INT.

  From the above I could conclude that the other RDBMS are
  generally representing the results of expressions in SELECTs as
  having one type out of a narrow set of types (in absence of
  explicit type casts.) This way, SQLite doesn't look much
  different to those.

  Or, to put it differently, I see the discussed problem as not
  being specific to SQLite.

The problem is that SQLite doesn't type columns, but the values which
are in them. Typical RDBMS implementations type the columns. IOW,
dynamic typing (SQLite) versus static typing (everything else).

This means that, with SQLite, you need at least one row of data to
determine the types. It also means that if a column is null for that
particular row, the column will appear to be null.

It also means that, for VARCHAR columns, you cannot determine the
maximum width that can be stored in the column.

> One issue is that SQLite doesn't validate the column types. It uses
> some heuristics to determine the affinity type, but ultimately it
> allows any sequence of words along with an optional size specifier,
> e.g.:

> CREATE TABLE foo ( i INT, t TEXT, b ROAST BEEF(99) );

  Of course. So, it becomes the user's responsibility to make
  sure that the definition of the table in question is compatible
  with the RDBMS to which it's copied.

  I guess that much the same way, once the table is defined to use
  any types specific to an RDBMS implementation, it cannot be
  readily copied to any other RDBMS.

  The only SQLite-specific problem that I see there is that it
  doesn't actually treat the type name as a constraint on the
  values put into that field. But then, ``garbage in, garbage
  out'' is the guiding principle.

Yep. This isn't a problem for tables which are created through the
DBMI, as we can ensure that it accepts whatever it creates. The
problem arises if the user creates or modifies tables externally.

The other problem is that various DBMI drivers and clients don't
understand TEXT fields. Although that isn't inherently specific to the
SQLite driver, it does bring up the issue is how the driver should
treat fields whose decltype isn't recognised (given that, ultimately,
all SQLite columns are effectively TEXT columns).

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

[neteler]

On Mon, Jul 7, 2008 at 8:34 PM, Glynn Clements <glynn@gclements.plus.com> wrote:
...

It also means that, for VARCHAR columns, you cannot determine the
maximum width that can be stored in the column.

But doesn't v.in.ascii scan for column lengths in any case?
Maybe make that switchable and use the extracted info?

in point.c:
/* Analyse points ascii file. Determine number of columns and column types.
* ascii_tmp: write copy of tempfile to ascii_tmp:
* rowlength: maximum row length
* ncolumns: number of columns
* minncolumns: minimum number of columns
* column_type: column types
* column_length: column lengths (string only)
*/

int points_analyse(FILE * ascii_in, FILE * ascii, char *fs,
                   int *rowlength, int *ncolumns, int *minncolumns,
                   int **column_type, int **column_length, int skip_lines,
                   int xcol, int ycol, int region_flag)

Markus

[Glynn_Clements1]

Markus Neteler wrote:

> It also means that, for VARCHAR columns, you cannot determine the
> maximum width that can be stored in the column.

But doesn't v.in.ascii scan for column lengths in any case?

It does.

Maybe make that switchable and use the extracted info?

If you don't use the columns= option, v.in.ascii uses the column
lengths determined from the data.

However, if you use the columns= option, it uses the answer to create
the table, then validates the table against the actual data.

So, the problem arises because v.in.geonames tell v.in.ascii to use
varchar(4000) for the "alternatename" column, but once the table has
been created the SQLite driver reports the column as varchar(255).

Essentially, db/drivers/sqlite/describe.c needs to do a better job of
parsing the result from sqlite3_column_decltype().

At present, it parses the decltype to an SQLite affinity type:

    int aff = SQLITE_FLOAT;

  ...

    if ( strstr(lc,"int") )
    {
        aff = SQLITE_INTEGER;
    }
    else if ( strstr(lc,"char") || strstr(lc,"clob")
              || strstr(lc,"text") || strstr(lc,"date") )
    {
        aff = SQLITE_TEXT;
    }
    else if ( strstr(lc,"blob") )
    {
        aff = SQLITE_BLOB;
    }

then converts the affinity type to a DBMI type.

[I'm not sure why the default is float.]

What it should do (IMHO) is to parse the decltype directly to a DBMI
type (including parsing VARCHAR() types, to determine the size). It
should only use the affinity type if there is no decltype (which
occurs when a column in a SELECT statement is an expression rather
than a column reference).

But db/drivers/sqlite/create_table.c also needs to create the columns
with the correct type in the first place. At present, it "condenses"
the type down to one of SQLite's affinity types (e.g. VARCHAR columns
are created as TEXT, as are TIME and DATE).

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

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

[...]

>> From the above I could conclude that the other RDBMS are generally
>> representing the results of expressions in SELECTs as having one
>> type out of a narrow set of types (in absence of explicit type
>> casts.) This way, SQLite doesn't look much different to those.

>> Or, to put it differently, I see the discussed problem as not being
>> specific to SQLite.

> The problem is that SQLite doesn't type columns, but the values which
> are in them. Typical RDBMS implementations type the columns. IOW,
> dynamic typing (SQLite) versus static typing (everything else).

> This means that, with SQLite, you need at least one row of data to
> determine the types. It also means that if a column is null for that
> particular row, the column will appear to be null.

  ... Thus requiring up to the whole set of records to be fetched
  in order to determine the ``column type''. And if the field is
  NULL for all the records, the only sensible behaviour would be
  to signal an error.

> It also means that, for VARCHAR columns, you cannot determine the
> maximum width that can be stored in the column.

  When retrieving data, the CHAR () and VARCHAR () columns should
  be treated as TEXT. When storing data, it may be sensible to
  issue a warning (or error) if the value doesn't fit into such a
  ``declared'' constraint.

>>> One issue is that SQLite doesn't validate the column types. It uses
>>> some heuristics to determine the affinity type, but ultimately it
>>> allows any sequence of words along with an optional size specifier,
>>> e.g.:

>>> CREATE TABLE foo ( i INT, t TEXT, b ROAST BEEF(99) );

>> Of course. So, it becomes the user's responsibility to make sure
>> that the definition of the table in question is compatible with the
>> RDBMS to which it's copied.

>> I guess that much the same way, once the table is defined to use any
>> types specific to an RDBMS implementation, it cannot be readily
>> copied to any other RDBMS.

>> The only SQLite-specific problem that I see there is that it doesn't
>> actually treat the type name as a constraint on the values put into
>> that field. But then, ``garbage in, garbage out'' is the guiding
>> principle.

> Yep. This isn't a problem for tables which are created through the
> DBMI, as we can ensure that it accepts whatever it creates. The
> problem arises if the user creates or modifies tables externally.

  Then it becomes the user's responsibility not to exploit this
  SQLite-specific feature (or, otherwise, be ready to solve any
  inconsistencies that may arise.)

[...]

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

[...]

> So, the problem arises because v.in.geonames tell v.in.ascii to use
> varchar(4000) for the "alternatename" column, but once the table
> has been created the SQLite driver reports the column as
> varchar(255).

> Essentially, db/drivers/sqlite/describe.c needs to do a better job
> of parsing the result from sqlite3_column_decltype().

> At present, it parses the decltype to an SQLite affinity type:

> int aff = SQLITE_FLOAT;
  
> ...
  
> if ( strstr(lc,"int") )
> {
> aff = SQLITE_INTEGER;
> }
> else if ( strstr(lc,"char") || strstr(lc,"clob")
> || strstr(lc,"text") || strstr(lc,"date") )
> {
> aff = SQLITE_TEXT;
> }
> else if ( strstr(lc,"blob") )
> {
> aff = SQLITE_BLOB;
> }

> then converts the affinity type to a DBMI type.

> [I'm not sure why the default is float.]

--cut: http://www.sqlite.org/datatype3.html--
2.1 Determination Of Column Affinity

   The type affinity of a column is determined by the declared type of
   the column, according to the following rules:
    1. If the datatype contains the string "INT" then it is assigned
       INTEGER affinity.
    2. If the datatype of the column contains any of the strings "CHAR",
       "CLOB", or "TEXT" then that column has TEXT affinity. Notice that
       the type VARCHAR contains the string "CHAR" and is thus assigned
       TEXT affinity.
    3. If the datatype for a column contains the string "BLOB" or if no
       datatype is specified then the column has affinity NONE.
    4. If the datatype for a column contains any of the strings "REAL",
       "FLOA", or "DOUB" then the column has REAL affinity
    5. Otherwise, the affinity is NUMERIC.
--cut: http://www.sqlite.org/datatype3.html--

> What it should do (IMHO) is to parse the decltype directly to a
> DBMI type (including parsing VARCHAR() types, to determine the
> size). It should only use the affinity type if there is no decltype
> (which occurs when a column in a SELECT statement is an expression
> rather than a column reference).

  For the text data fields, it may make sense to report TEXT
  instead of the declared type (provided that the callers do
  support that type.)

[...]

[Glynn_Clements1]

Ivan Shmakov wrote:

>> The only SQLite-specific problem that I see there is that it doesn't
>> actually treat the type name as a constraint on the values put into
>> that field. But then, ``garbage in, garbage out'' is the guiding
>> principle.

> Yep. This isn't a problem for tables which are created through the
> DBMI, as we can ensure that it accepts whatever it creates. The
> problem arises if the user creates or modifies tables externally.

  Then it becomes the user's responsibility not to exploit this
  SQLite-specific feature (or, otherwise, be ready to solve any
  inconsistencies that may arise.)

It isn't just about "SQLite-specific" issues.

The question is whether sqlite/describe.c only needs to understand the
types which sqlite/create_table.c itself uses, or whether it needs to
understand all of the "standard" types (i.e. anything defined by one
of the SQL standards, as well as any additional types which are
supported by "popular" RDBMSes).

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

[Glynn_Clements1]

Helena Mitasova wrote:

(I am sending this off-list but feel free to post
it with your answer if you think it should
go to the developers list)

First of all thank you very much for your help with r.series,
the paper where we have used it extensively got accepted
without any need for revision and is now in press for Journal
of Coastal Research.

I have a few questions related to recent changes in flow routing
modules:

- Is the latest fix #197 submitted by Andy Danner for
r.terraflow good enough now?

I can't comment on the fixes to the SFD support, as I don't understand
r.terraflow enough. However, it does resolve the issues which I had
with the original patch going to unnecessary lengths to eliminate
warnings.

If yes, would you be so kind and submit

I'll do that as soon as I check that it still compiles (I haven't
checked since making all of the "struct Option" changes which were
triggered by this issue).

it before Will moves r.terraflow into the iostream directory as
suggested by Paul.

Can you provide details?

- I haven't yet updated my trunk to the latest
grass7 but I noticed that you say that simwe is
severely broken. I have been using
it a lot over the past 2 years and Yann has cleaned up the code
a little bit few months ago so I am wondering whether we have
messed it up or it has been broken from the start and I just
don't use the broken options. The site's related code
has not been updated properly so that may be causing
problems (I can comment it out - it is not absolutely needed,
although it is nice to have for outputs like this
http://skagit.meas.ncsu.edu/~helena/wrriwork/balsam/fan
http://skagit.meas.ncsu.edu/~helena/gmslab/gisc00/duality.html
in addition to the raster series)

It is the sites code which is the problem.

I have recently fixed the lib/sites code to use "struct Map_info *"
for the "handle" to a sites "file", rather than continuing to pretend
that it's a "FILE *".

This change was mostly straightforward, but it did show up a couple of
issues with simwe. Specifically:

1. The code is confused about "fw". input_data() in simlib/input.c
opens it with fopen(), main_loop() in simlib/hydro.c writes textual
data to it with fwrite(), but main() in r.sim.sediment/main.c closes
it with G_sites_close() (which requires a "struct Map_info *").

2. fdoutwalk never gets closed. output_data() in simlib/output.c opens
it on demand, and that function gets called repeatedly from the
modules. When sites "files" were just files, they didn't need to be
explicitly closed, as all files are closed automatically upon exit.
However, vector maps actually need to be closed (G_sites_close() calls
Vect_build()).

Point 1 probably isn't a problem; it appears that the G_sites_close()
call just needs to be changed to fclose() as is done by r.sim.water.

Point 2 is somewhat harder, due to:

I know that the code is ugly

For this reason, I gave up trying to understand how to fix the code,
and just disabled the module.

- as it is (similarly as v.surf.rst) a crude rewrite of our fortran
code

My first thought on looking at the code was "this looks like fortran".

My second thought was "why does each module have its own copy of
waterglobs.h, when it's essential that both the library and the
modules have consistent definitions?"

- but maybe with some help
we can make it acceptable for keeping it in GRASS?

The fundamental problem with the code is the lack of modularity.

There are roughly 140 global variables (which are actually created by
each module and referenced by the library, which is backwards), yet
relatively few file-local variables and function arguments. This makes
it practically impossible to determine the consequences of any change
without analysing the entire code of the library and both modules.

At least a few of those variables are only used within a single
function (e.g. fdsfile is never used outside of input_data), so should
be local variables.

Ideally, a library should neither define nor use *any* global
variables[1]; all data should be passed and returned using function
arguments. If this is impractical due to the number of arguments[2],
the next best thing is to make variables local to a file containing
the functions which use them, or to bundle associated variables into a
structure (but don't over-do this, otherwise it just shifts the problem
from needing to check the entire source for variable references to
needing to check the entire source for structure field references).

[1] Although GRASS itself deviates from this ideal quite often, with
the end result that the libraries aren't much use for anything except
typical GRASS-style command-line modules.

[2] For an example of what isn't practical, see IL_init_params_2d(). I
have been seeing this warning for as long as I can remember:

main.c:643: warning: passing arg 41 of `IL_init_params_2d' from incompatible pointer type

Argument 41 is ... lets see ... one, two, three, fou... ah, never
mind, I'll just leave it.

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

[Paul_Kelly]

On Wed, 9 Jul 2008, Glynn Clements wrote:

it before Will moves r.terraflow into the iostream directory as
suggested by Paul.

Can you provide details?

Will is working on a new r.viewshed module which uses the same iostream C++ library as r.terraflow. To avoid code duplication I suggested this layout:
Have a subdirectory:
raster/iostream/
and within that lib/ r.terraflow/ r.viewshed/
One copy of the library in the lib directory - it compiles into a static library which is then linked in by r.terraflow and r.viewshed.

This is subject to revision/better ideas of course - when Will has the r.viewshed code ready (I haven't seen it) maybe we can experiment with it in grass-addons in the first instance.

Paul

[Glynn_Clements1]

Paul Kelly wrote:

>> it before Will moves r.terraflow into the iostream directory as
>> suggested by Paul.
>
> Can you provide details?

Will is working on a new r.viewshed module which uses the same iostream
C++ library as r.terraflow. To avoid code duplication I suggested this
layout:
Have a subdirectory:
raster/iostream/
and within that lib/ r.terraflow/ r.viewshed/
One copy of the library in the lib directory - it compiles into a static
library which is then linked in by r.terraflow and r.viewshed.

This is subject to revision/better ideas of course - when Will has the
r.viewshed code ready (I haven't seen it) maybe we can experiment with
it in grass-addons in the first instance.

I suggest moving the iostream library to lib/iostream, rather than
grouping r.terraflow and r.viewshed together.

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

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

[...]

>>> Yep. This isn't a problem for tables which are created through the
>>> DBMI, as we can ensure that it accepts whatever it creates. The
>>> problem arises if the user creates or modifies tables externally.

>> Then it becomes the user's responsibility not to exploit this
>> SQLite-specific feature (or, otherwise, be ready to solve any
>> inconsistencies that may arise.)

> It isn't just about "SQLite-specific" issues.

  Indeed.

> The question is whether sqlite/describe.c only needs to understand
> the types which sqlite/create_table.c itself uses, or whether it
> needs to understand all of the "standard" types (i.e. anything
> defined by one of the SQL standards, as well as any additional types
> which are supported by "popular" RDBMSes).

  On the one hand, it certainly makes sense to try to obtain the
  most complete description from the source database when
  replicating data. On the other hand, it isn't completely
  necessary and may even be harmful (consider, e. g., an
  application which allocates static N-character string storage
  for reading VARCHAR (N) data from SQLite.)

  Therefore, it may make sense to provide both and let the
  developer decide.

[Glynn_Clements1]

Glynn Clements wrote:

What it should do (IMHO) is to parse the decltype directly to a DBMI
type (including parsing VARCHAR() types, to determine the size). It
should only use the affinity type if there is no decltype (which
occurs when a column in a SELECT statement is an expression rather
than a column reference).

But db/drivers/sqlite/create_table.c also needs to create the columns
with the correct type in the first place. At present, it "condenses"
the type down to one of SQLite's affinity types (e.g. VARCHAR columns
are created as TEXT, as are TIME and DATE).

I have made these changes locally (patch attached).

Can someone suggest some test cases?

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

(attachments)

sqlite-types.diff (17.4 KB)

[Ivan_Shmakov1]

Glynn Clements <glynn@gclements.plus.com> writes:

[...]

> I have made these changes locally (patch attached).

  BTW, as some of the changes are actually plain clean-ups, could
  they be considered for an ``immediate'' commit?

[...]

> Index: db/drivers/sqlite/proto.h
> ===================================================================
> --- db/drivers/sqlite/proto.h (revision 32073)
> +++ db/drivers/sqlite/proto.h (working copy)
> @@ -1,9 +1,11 @@
> #ifndef __SQLITE_PROTO_H__
> #define __SQLITE_PROTO_H__

> +#include "globals.h"
> +
> /* error.c */
> void init_error ( void );
> -void append_error ( char * );
> +void append_error ( const char * );
> void report_error ( void );

> /* cursor.c */

[...]

> Index: db/drivers/sqlite/main.c
> ===================================================================
> --- db/drivers/sqlite/main.c (revision 32073)
> +++ db/drivers/sqlite/main.c (working copy)
> @@ -11,12 +11,14 @@
> * for details.
> *
> **************************************************************/
> -#define MAIN
> +
> #include <stdlib.h>
> #include <grass/dbmi.h>
> #include "globals.h"
> #include "dbdriver.h"

> +sqlite3 *sqlite;
> +
> int main (int argc, char *argv)

> {
> Index: db/drivers/sqlite/globals.h
> ===================================================================
> --- db/drivers/sqlite/globals.h (revision 32073)
> +++ db/drivers/sqlite/globals.h (working copy)
> @@ -1,5 +1,8 @@
> #include <sqlite3.h>

> +#ifndef DBMI_SQLITE_PROTO_H
> +#define DBMI_SQLITE_PROTO_H
> +
> /* cursors */
> typedef struct _cursor {
> sqlite3_stmt *statement;
> @@ -12,12 +15,7 @@

> } cursor;

> -#ifdef MAIN
> - sqlite3 *sqlite;
> - dbString *errMsg = NULL; /* error message */
> -#else
> - extern sqlite3 *sqlite;
> - extern dbString *errMsg;
> -#endif
> +extern sqlite3 *sqlite;

> +#endif

> Index: db/drivers/sqlite/error.c
> ===================================================================
> --- db/drivers/sqlite/error.c (revision 32073)
> +++ db/drivers/sqlite/error.c (working copy)
> @@ -14,8 +14,11 @@
> #include <stdio.h>
> #include <grass/gis.h>
> #include <grass/dbmi.h>
> +#include "proto.h"
> #include "globals.h"

> +static dbString *errMsg = NULL; /* error message */
> +
> /* init error message */
> void
> init_error ( void )
> @@ -30,7 +33,7 @@

> /* append error message */
> void
> -append_error ( char *msg )
> +append_error ( const char *msg )
> {
> db_append_string ( errMsg, msg);
> }

[Glynn_Clements1]

Ivan Shmakov wrote:

> I have made these changes locally (patch attached).

  BTW, as some of the changes are actually plain clean-ups, could
  they be considered for an ``immediate'' commit?

Committed as r32080.

A new version of the patch, against the updated code, is attached.

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

(attachments)

sqlite-types.2.diff (15.5 KB)

[Glynn_Clements1]

Glynn Clements wrote:

> > I have made these changes locally (patch attached).
>
> BTW, as some of the changes are actually plain clean-ups, could
> they be considered for an ``immediate'' commit?

Committed as r32080.

A new version of the patch, against the updated code, is attached.

Committed.

[It didn't look as if anyone was going to test it otherwise.]

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

[neteler]

On Fri, Jul 18, 2008 at 7:25 PM, Glynn Clements
<glynn@gclements.plus.com> wrote:
...

A new version of the patch, against the updated code, is attached.

Committed.

[It didn't look as if anyone was going to test it otherwise.]

I have tested it now - it works nicely. I also copied back and forth to
PostgreSQL and didn't come across problems.

Backported to GRASS 6.4.svn as well.

Thanks for your efforts, Glynn and Ivan!

Markus

[Paul_Kelly]

On Thu, 10 Jul 2008, Glynn Clements wrote:

Paul Kelly wrote:

it before Will moves r.terraflow into the iostream directory as
suggested by Paul.

Can you provide details?

Will is working on a new r.viewshed module which uses the same iostream
C++ library as r.terraflow. To avoid code duplication I suggested this
layout:
Have a subdirectory:
raster/iostream/
and within that lib/ r.terraflow/ r.viewshed/
One copy of the library in the lib directory - it compiles into a static
library which is then linked in by r.terraflow and r.viewshed.

This is subject to revision/better ideas of course - when Will has the
r.viewshed code ready (I haven't seen it) maybe we can experiment with
it in grass-addons in the first instance.

I suggest moving the iostream library to lib/iostream, rather than
grouping r.terraflow and r.viewshed together.

I've made this change now in svn trunk, in preparation for adding r.viewshed to grass-addons and sharing the iostream library with r.terraflow. The iostream library had quite a lot of include files; I've put these in include/iostream rather than polluting the main GRASS include/ directory. I wasn't sure which ones were internal to the library and which ones are required by user programs, so have just included them all in include/ (that was the way it was done in the IOStream subdirectory of r.terraflow).

r.terraflow still compiles fine so I hope there will be no unforeseen consequences.

Paul

[Maciej_Sieczka2]

Paul Kelly pisze:

r.terraflow still compiles fine so I hope there will be no unforeseen consequences.

In the current develbranch6 (r32655) there are 2 errors at the build time:

in lib/iostream:

$ make
c++ -I/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include -pipe -march=core2 -g -Wall -D_FILE_OFFSET_BITS=64 -DPACKAGE=\""grasslibs"\" -I/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include -o OBJ.x86_64-unknown-linux-gnu/ami_stream.o -c ami_stream.cc
In file included from ami_stream.cc:29:
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h: In constructor 'AMI_STREAM<T>::AMI_STREAM(const char*, AMI_stream_type)':
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:256: error: there are no arguments to 'strcpy' that depend on a template parameter, so a declaration of 'strcpy' must be available
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:256: error: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h: In member function 'AMI_err AMI_STREAM<T>::name(char**)':
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:373: error: there are no arguments to 'strlen' that depend on a template parameter, so a declaration of 'strlen' must be available
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:374: error: there are no arguments to 'strcpy' that depend on a template parameter, so a declaration of 'strcpy' must be available
ami_stream.cc: At global scope:
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
ami_stream.cc:46: warning: deprecated conversion from string constant to 'char*'
make: *** [OBJ.x86_64-unknown-linux-gnu/ami_stream.o] Error 1

in raster/r.terraflow:

$ make
c++ -c -I/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include -pipe -march=core2 -g -Wall -DUSER=\"shoofi\" -DNODATA_FIX -D_FILE_OFFSET_BITS=64 -DPACKAGE=\""grassmods"\" -DELEV_FLOAT common.cc -o OBJ.x86_64-unknown-linux-gnu/FLOAT/common.o
In file included from /home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami.h:27,
                  from common.h:27,
                  from common.cc:35:
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h: In constructor 'AMI_STREAM<T>::AMI_STREAM(const char*, AMI_stream_type)':
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:256: error: there are no arguments to 'strcpy' that depend on a template parameter, so a declaration of 'strcpy' must be available
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:256: error: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h: In member function 'AMI_err AMI_STREAM<T>::name(char**)':
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:373: error: there are no arguments to 'strlen' that depend on a template parameter, so a declaration of 'strlen' must be available
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_stream.h:374: error: there are no arguments to 'strcpy' that depend on a template parameter, so a declaration of 'strcpy' must be available
In file included from /home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_sort_impl.h:25,
                  from /home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami_sort.h:23,
                  from /home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/ami.h:33,
                  from common.h:27,
                  from common.cc:35:
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/mem_stream.h: In member function 'AMI_err MEM_STREAM<T>::name(char**)':
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/mem_stream.h:90: error: there are no arguments to 'strlen' that depend on a template parameter, so a declaration of 'strlen' must be available
/home/shoofi/src/straight/grass64/dist.x86_64-unknown-linux-gnu/include/grass/iostream/mem_stream.h:91: error: there are no arguments to 'strcpy' that depend on a template parameter, so a declaration of 'strcpy' must be available
make: *** [OBJ.x86_64-unknown-linux-gnu/FLOAT/common.o] Error 1

Maciek

--
Maciej Sieczka
www.sieczka.org