db_sqlite branch review request
Hi all, The db_sqlite module is now getting ready for merge to master... It would be nice if it'd get some scrutiny from other developer before the merge, though. So far I've now addressed the earlier comments based on the original patch, added the docbook notes / README, Natanael Copa contributed the kamdbctl related work, and Henning Westerholt did the automation of the SQL script generation from the XML schema. Unless there's major things, I'm planning to merge with master late this week. Thanks, Timo
On Monday 30 May 2011, Timo Teräs wrote:
The db_sqlite module is now getting ready for merge to master... It would be nice if it'd get some scrutiny from other developer before the merge, though.
Hi Timo, I've had a quick look, just noticed some smaller things: You implement your own str handling functions (str_dup), there are some already defined in ut.h file ([pkg,shm]_str_dup), maybe they are suitable for you. You've added some documentation in the files for the database functions, maybe you can convert them to the doxygen format (have a look to the /lib/srdb1/* files for examples, its should be not that difficult). In your GPL headers, you use the * $Id$ macro, this is not really necessary anymore in new modules as its not evaluated from git, maybe you can remove this. One question related to the db_sqlite_raw_query function, does sqlite really support arbitrary SQL functions? For example does is support SELECT DISTINCT, which is one query that the cr module uses. Cheers, Henning
On 06/01/2011 01:35 PM, Henning Westerholt wrote:
On Monday 30 May 2011, Timo Teräs wrote:
The db_sqlite module is now getting ready for merge to master... It would be nice if it'd get some scrutiny from other developer before the merge, though.
Hi Timo,
I've had a quick look, just noticed some smaller things:
You implement your own str handling functions (str_dup), there are some already defined in ut.h file ([pkg,shm]_str_dup), maybe they are suitable for you.
Those are slightly different. The str_dup and str_assign I have, take in a C-string, or a pointer+length. Where as the pkg_str_dup from ut.h takes const str* as source. I did try to look if suitable functions already exists but could not find with quick search. If there's one, let me know :)
You've added some documentation in the files for the database functions, maybe you can convert them to the doxygen format (have a look to the /lib/srdb1/* files for examples, its should be not that difficult).
I think the comments were modelled mostly after the original database module which I used as starting point. It didn't have any doxygen stuff so I didn't realise it's standard. I'll take a look at this.
In your GPL headers, you use the * $Id$ macro, this is not really necessary anymore in new modules as its not evaluated from git, maybe you can remove this.
Also leftover from the older module. Will remove.
One question related to the db_sqlite_raw_query function, does sqlite really support arbitrary SQL functions? For example does is support SELECT DISTINCT, which is one query that the cr module uses.
I thought the idea is to just pass the stuff to SQL interpreter so we can use whatever the database supports or not. SELECT DISTINCT is supported according to http://www.sqlite.org/lang_select.html - Timo
On Wednesday 01 June 2011, Timo Teräs wrote:
You implement your own str handling functions (str_dup), there are some already defined in ut.h file ([pkg,shm]_str_dup), maybe they are suitable for you.
Those are slightly different. The str_dup and str_assign I have, take in a C-string, or a pointer+length. Where as the pkg_str_dup from ut.h takes const str* as source. I did try to look if suitable functions already exists but could not find with quick search. If there's one, let me know :)
Hi Timo, I think they are some more in lib/cds/dstring.h or so - but they are mainly used in presence modules, i think - not sure if they really fit.
You've added some documentation in the files for the database functions, maybe you can convert them to the doxygen format (have a look to the /lib/srdb1/* files for examples, its should be not that difficult).
I think the comments were modelled mostly after the original database module which I used as starting point. It didn't have any doxygen stuff so I didn't realise it's standard. I'll take a look at this.
Well, standard its maybe a bit too much, but already a lot of modules use this: http://sip-router.org/doxygen/sip-router/branch/master/index.html
[..]
One question related to the db_sqlite_raw_query function, does sqlite really support arbitrary SQL functions? For example does is support SELECT DISTINCT, which is one query that the cr module uses.
I thought the idea is to just pass the stuff to SQL interpreter so we can use whatever the database supports or not.
SELECT DISTINCT is supported according to http://www.sqlite.org/lang_select.html
Yes, this is the idea. But I just wanted to ask because otherwise it would break for people if you set this flag and the backend then not supports it. :-) Best regards, Henning
participants (2)
-
Henning Westerholt -
Timo Teräs