Skip to content

Aggregate functions (finally!). #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Dec 26, 2014
Merged

Aggregate functions (finally!). #54

merged 15 commits into from
Dec 26, 2014

Conversation

Sean1708
Copy link
Contributor

@Sean1708 Sean1708 commented Dec 7, 2014

This is definitely far from finished but the good news is, it works. The bad news is, the implementation is horrifically vile, basically a huge hack full of unsafe_copy!s to get round the garbage collector. I'll keep working on it over the next couple of days but I thought I'd let you have a look, see if anything jumps out at you.

The basic idea is that an integer and a pointer are stored contiguously in the buffer returned by sqlite3_aggregate_context. The pointer points to a bytearray (allocated with c_malloc so that the garbage collector doesn't free it when the step function finishes executing) which contains the serialized value returned from the user's step function. The integer just stores the length of this bytearray.

As I say, this is just a working prototype, I'll try neating it up over the next couple of days.

@Sean1708
Copy link
Contributor Author

Sean1708 commented Dec 7, 2014

And that test has just shown how fragile this implementation is.

No arguments are ever passed to the final function so values was a void pointer
causing sqlite3_value_type to fail. This simply stopped trying to collect args.

Also changed the order of the function definitions to something that made more
sense.
No measurable speed-up but slightly simplifies the implementation.
Attempt to account for big-endianness.
Don't call sqlserialize unnecessarily.
Try to avoid memory-leaks.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.88%) when pulling fb49265 on aggregates into 3e1d14d on master.

And therefore default the name to the step functions name.

README and tests have also been updated.
No performance change but reduces bytes allocated by ~5%.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.88%) when pulling 685606d on aggregates into 3e1d14d on master.

@Sean1708
Copy link
Contributor Author

I'm still not enamoured by this but I think I've made all the changes that I can. I'm happy for it to be merged once you've checked it over.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) when pulling fa7742d on aggregates into 3e1d14d on master.

The only unknown is the users function, any other exceptions are bugs in
SQLite.jl and should be fixed not hidden away.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) when pulling 57c19ca on aggregates into 3e1d14d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.32%) when pulling ef1d1a9 on aggregates into 3e1d14d on master.

@quinnj
Copy link
Member

quinnj commented Dec 16, 2014

This code is wild. The tests we have already look great though. I don't mind merging, but I have to admit that I haven't quite grokked the entirety of what's going on. I might have some more time in the next few days, but it may not be until the weekend or early next week to really dig in some more.

@Sean1708
Copy link
Contributor Author

There might be some edge cases I've forgotten about but I've tried pretty hard to break it and it's not blown up in my face yet.

@quinnj
Copy link
Member

quinnj commented Dec 21, 2014

I guess my one thing is I'm always a little bummed if a function has to use a try-catch block because they can be such performance killers in sensitive code, and usually there's a way to restructure the code to avoid, especially in Julia; though obviously sometimes they're necessary. I say we merge, tag a release, and then can continue polishing things going forward. Feel free to do the honors, or I can do it in the next day or two and tag a release.

@Sean1708
Copy link
Contributor Author

I know you do which is why I made them as small as possible. In this case though, if they weren't there any exceptions thrown by the user's function would leak memory, which I would consider a far greater sin. I'll keep having a poke about though, see what I can do.

Sean1708 added a commit that referenced this pull request Dec 26, 2014
@Sean1708 Sean1708 merged commit 6eb988a into master Dec 26, 2014
@quinnj
Copy link
Member

quinnj commented Dec 27, 2014

+1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants