Skip to content
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

Window/Aggregate function implementation -- please review #14

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Buggaboo
Copy link

@Buggaboo Buggaboo commented Aug 30, 2023

So, I took one of sqlite3's sumint example to develop the window function, because I'm planning on using this codebase to develop other interesting window functions.

I'm not so super experienced with rust yet, so I would appreciate if you could give me some pointers.

I have two issues:

  1. double free issue in test_sum_int.rs, I suspect my 'get_aggregate_context_value' somehow took ownership.
  2. can't load the plugin: Error: dlsym(0x808c4010, sqlite3_sumint_init): symbol not found, I can't load the plugin, I might have missed something.

1. double free issue in test_sum_int.rs
2. can't load the plugin: Error: dlsym(0x808c4010, sqlite3_sumint_init): symbol not found
@asg017
Copy link
Owner

asg017 commented Aug 30, 2023

double free issue in test_sum_int.rs, I suspect my 'get_aggregate_context_value' somehow took ownership.

I'm not 100% sure, but x_final_wrapper and x_value_wrapper might need to reclaim the b box with Box::into_raw() , otherwise I think rust will drop it for you. Not too sure if it matter here, just saw it while skimming

can't load the plugin: Error: dlsym(0x808c4010, sqlite3_sumint_init): symbol not found, I can't load the plugin, I might have missed something.

On this line, change sqlite3_sum_int_init to sqlite3_sumint_init

pub fn sqlite3_sum_int_init(db: *mut sqlite3) -> Result<()> {

Also, sorry if this was your first foray into Rust - this codebase isn't that great, and Rust <-> C FFI is never fun!

On the other hand, this might inspire me to get proper aggregate/window support is this project...

2. sqlite3, has a naming convention: sqlite3_xxx_init, where xxx cannot contain underscores
   although the function itself may contain an underscore in sqlite3 itself
@Buggaboo
Copy link
Author

Buggaboo commented Aug 31, 2023

Hi,

This is ready for review. I checked with valgrind, the new sumint example has no detectable leaks.

@Buggaboo Buggaboo force-pushed the feature/add-create-window-function branch from 9de6f9a to f272ccd Compare September 4, 2023 13:40
…algrind

aux implementation might not be necessary, due to aggregate context
@Buggaboo Buggaboo marked this pull request as ready for review September 4, 2023 13:52
@Buggaboo Buggaboo changed the title Help needed to get window function working *don't merge* Window (aggregate) function implementation Sep 4, 2023
@Buggaboo Buggaboo changed the title Window (aggregate) function implementation Window/Aggregate function implementation Sep 5, 2023
@Buggaboo Buggaboo changed the title Window/Aggregate function implementation Window/Aggregate function implementation -- please review Dec 11, 2023
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.

2 participants