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.

Sign up for GitHub

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

Jump to bottom

Limbo crashes on colab #494

Closed
franz101 opened this issue Dec 16, 2024 · 10 comments
Closed

Limbo crashes on colab #494

franz101 opened this issue Dec 16, 2024 · 10 comments
Labels
compat good first issue Good for newcomers help wanted Extra attention is needed

Comments

@franz101
Copy link

franz101 commented Dec 16, 2024 •
edited by jussisaurio
Loading

https://colab.research.google.com/drive/1llbFK082nh2JLDZ4Z-d0q-9j-bXtaCCE?usp=sharing
@jussisaurio
Copy link
Collaborator

jussisaurio commented Dec 16, 2024 •
edited
Loading 

VALUES (?, ?, ?)

SQL parameters are not implemented yet.

@jussisaurio jussisaurio changed the title Limbo crashes in Colab SQL parameter support Dec 16, 2024
@jussisaurio jussisaurio added help wanted Extra attention is needed good first issue Good for newcomers compat labels Dec 16, 2024
@jussisaurio
Copy link
Collaborator

jussisaurio commented Dec 16, 2024 •
edited
Loading

I changed the name of the issue to reflect the cause of the crash and edited your OP

edit: reverted, seems i was wrong about the cause

@krishvishal
Copy link
Contributor

krishvishal commented Dec 16, 2024 •
edited
Loading

I just looked at Colab notebook. It is crashing on conn.commit(). I think its because commit transaction is not implemented yet.

You can verify the crash by running it line by line here.

@jussisaurio jussisaurio changed the title SQL parameter support Limbo crashes on colab Dec 16, 2024
@jussisaurio
Copy link
Collaborator

ok seems i was too quick to jump to conclusions, just assumed

@franz101
Copy link
Author

ah yeah, I was curious about a simple POC

@LtdJorge

This comment was marked as off-topic.

Sign in to view
@LtdJorge LtdJorge mentioned this issue Dec 18, 2024
Closed
@LtdJorge
Copy link

@krishvishal was right, the panic is because it hits todo!() in commit() for the Python bindings, disregard my previous comment.

@amuldotexe
Copy link
Contributor

So perhaps the main task here is to gracefully handle this error & return a NotImplemented Error

Is that correct?

@LtdJorge
Copy link

So perhaps the main task here is to gracefully handle this error & return a NotImplemented Error

Is that correct?

Commit is not implemented on the Rust side either. I think it's best to wait for that.

amuldotexe added a commit to amuldotexe/limbo that referenced this issue Dec 20, 2024
@amuldotexe
gracefully handling errors for issue tursodatabase#494 , changes made…
f912771 
… 5 places where todo macros were replaced with relevant errors
@amuldotexe amuldotexe mentioned this issue Dec 20, 2024
Merged
penberg added a commit that referenced this issue Dec 21, 2024
@penberg
Merge 'Improve error reporting in Python bindings' from amuldotexe
307a7cb 
Solving issue #494   , changes made at 5 places where todo macros were
replaced with relevant errors to avoid the crashes _for now_

Reviewed-by: Avinash Sajjanshetty <[email protected]>

Closes #526
@franz101
Copy link
Author

Ok, we can close this one and create a new one / link commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compat good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@krishvishal @amuldotexe @franz101 @jussisaurio @LtdJorge