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

Implement create virtual table using vtab modules, more work on virtual tables #996

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

Conversation

PThorpe92
Copy link
Contributor

@PThorpe92 PThorpe92 commented Feb 13, 2025

This PR started out as one to improve the API of extensions but I ended up building on top of this quite a bit and it just kept going. Sorry this one is so large but there wasn't really a good stopping point, as it kept leaving stuff in broken states.

Opcodes Added

VCreate: Support for CREATE VIRTUAL TABLE t USING vtab_module

VUpdate: Support for INSERT and DELETE methods on virtual tables.

Sqlite uses xUpdate function with the VUpdate opcode to handle all insert/update/delete functionality in virtual tables..

This meant the extension API would be super confusing, as we would have to just document that:

if args[0] == NULL:  INSERT args[1] the values in args[2..]

if args[1] == NULL: DELETE args[0]

if args[0] != NULL && len(args) > 2: Update values=args[2..]  rowid=args[0] 

I know I asked @jussisaurio on discord about this already, but it just sucked so bad that I added some internal translation so we could expose a nice API and handle the logic ourselves while keeping with sqlite's opcodes.

I'll change it back if I have to, I just thought it was genuinely awful to have to rely on comments to explain all that to extension authors.

NOTE:

The included extension is not meant to be a legitimately useful one, it is there for testing purposes. I did something similar in #960 using a test extension, so I figure when they are both merged, I will go back and combine them into one since you can do many kinds at once, and that way it will reduce the amount of crates and therefore compile time.

Virtual table TODOS:

  1. Remaining opcodes.
  2. UPDATE (when we support the syntax)

@PThorpe92 PThorpe92 marked this pull request as draft February 13, 2025 22:35
@PThorpe92 PThorpe92 marked this pull request as ready for review February 13, 2025 22:37
start: 0,
stop: 0,
step: 0,
current: 0,
}
})
}

fn filter(cursor: &mut Self::VCursor, arg_count: i32, args: &[Value]) -> ResultCode {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why e.g. filter still returns a ResultCode and not a Result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that was tricky because the user is probably going to still have to manually return EOF in filters case, so doing Result<(), ResultCode> felt odd.

I have some changes to this anyway because I'm very close to having create virtual table t using x and VUpdate opcode both done, and those are changing this a bit too so I can prob just mark this as draft till then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so update on this... yeah I just didn't think it would be ideal to allow the possibility of Ok(ResultCode::Error) or vice versa, and the goal here is to have them return ResultCode::EOF, which doesn't really feel like it's an Err(ResultCode::EOF) or an Ok, if that makes any sense?

@PThorpe92 PThorpe92 marked this pull request as draft February 14, 2025 12:26
@PThorpe92 PThorpe92 changed the title Extensions: Improve API with better error handling, fix paths in proc macros Implement create virtual table using vtab modules, more work on virtual tables Feb 17, 2025
core/lib.rs Outdated Show resolved Hide resolved
core/lib.rs Outdated Show resolved Hide resolved
cli/app.rs Outdated Show resolved Hide resolved
core/ext/mod.rs Outdated Show resolved Hide resolved
@PThorpe92 PThorpe92 force-pushed the ext_cleanups branch 2 times, most recently from d1c0e10 to 9b79aa5 Compare February 17, 2025 16:00
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