Merge lp:~thisfred/u1db/get-index-keys-from-index into lp:u1db

Proposed by Eric Casteleijn on 2012-04-25
Status: Merged
Merged at revision: 259
Proposed branch: lp:~thisfred/u1db/get-index-keys-from-index
Merge into: lp:u1db
Diff against target: 0 lines
To merge this branch: bzr merge lp:~thisfred/u1db/get-index-keys-from-index
Reviewer Review Type Date Requested Status
John A Meinel (community) 2012-04-27 Approve on 2012-04-27
Roberto Alsina (community) 2012-04-25 Approve on 2012-04-26
Review via email: mp+103546@code.launchpad.net

Commit Message

Implemented get_index_keys in all 3 backends.

Description of the Change

Implemented get_index_keys in all 3 backends.

To post a comment you must log in.
259. By Eric Casteleijn on 2012-04-25

don't lie to me mr. docstring.

Roberto Alsina (ralsina) wrote :

Looks good to me

review: Approve
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 4/27/2012 2:31 PM, Eric Casteleijn wrote:
> You have been requested to review the proposed merge of
> lp:~thisfred/u1db/get-index-keys-from-index into lp:u1db.
>
> For more details, see:
> https://code.launchpad.net/~thisfred/u1db/get-index-keys-from-index/+merge/103546
>
> Implemented get_index_keys in all 3 backends.
>
> It returns a list of (key, frequency) tuples, since we can get this
> info from sqlite cheaply anyway, and it's useful for tag clouds
> etc.
>

+int
+u1db_get_index_keys(u1database *db, char *index_name,
+ void *context, u1db_key_callback cb)
+{
+ int frequency;
+ int status = U1DB_OK;
+ char *key = NULL;
+ sqlite3_stmt *statement;
+ status = sqlite3_prepare_v2(

You are doing "prepare" of the statement, but you aren't finalizing it
in the 'finish' block. You'll want to add that to avoid a memory leak.

+ while (status == SQLITE_ROW) {
+ key = (char*)sqlite3_column_text(statement, 0);
+ frequency = (int)sqlite3_column_int(statement, 1);
+ cb(context, key, frequency);
+ status = sqlite3_step(statement);
+ }

I think we should be checking the integer return value from 'cb' and
propagating it as a failure if it isn't U1DB_OK. Otherwise failures in
a cb will get silently suppressed. (I realize that isn't done
everywhere in the code, but we should.)

I'm a little hesitant to expose the count as part of the API, even if
we can get it cheaply today. Can we get it cheaply from Cassandra?

I would tend to expose less until it is found to be useful.

So some tweaks, but I think worth adding.

John
=:->

 review: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+alhAACgkQJdeBCYSNAANTngCg2UzA2UAZftEgFVn/WajS4aHZ
mCQAnjEQq03UWqEmihS82Wuh1s9p+6QO
=qf9L
-----END PGP SIGNATURE-----

review: Approve
260. By Eric Casteleijn on 2012-04-27

added missing finalize

261. By Eric Casteleijn on 2012-04-27

deyagnification

Preview Diff

Empty

Subscribers

People subscribed via source and target branches