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

Add support for DELETE query planning #538

Merged
merged 14 commits into from
Dec 27, 2024

Conversation

seonWKim
Copy link
Contributor

@seonWKim seonWKim commented Dec 22, 2024

Purpose of this PR

Support for DELETE query execution planning

Implementation Details

The main entry point for planning DELETE queries is the translate_delete function. It is composed of three primary steps:

  • prepare_delete_plan:

    • Reuses the existing SELECT query's WHERE clause parsing logic to interpret and construct the initial delete plan.
  • optimize_delete_plan:

    • eliminating BETWEEN expressions
    • usage of indexes
  • emit_program_for_delete:

    • Add instructions for delete operation

I've tried to reuse existing logic(mostly on SELECT operations) as much as I can, so that we can automatically incorporate new changes automatically.

Delete planning debug

I've used println!(...) to specify the rows to delete. Example below
image

Bytecode compatibility

EXPLAIN DELETE FROM users WHERE id = 1;

image

EXPLAIN DELETE FROM users WHERE id > 3

image

EXPLAIN DELETE FROM users WHERE id < 3

image

TODO(future works)

  • Add support for Clear opcode
  • Add test when delete is implemented in Cursor
image

@seonWKim seonWKim force-pushed the feature/delete-planning branch from a55b0d4 to a42b185 Compare December 22, 2024 05:22
@seonWKim seonWKim force-pushed the feature/delete-planning branch from 3d2ac33 to 57c7a56 Compare December 22, 2024 05:27
@seonWKim seonWKim force-pushed the feature/delete-planning branch from f46fcee to e83819e Compare December 22, 2024 07:00
@seonWKim seonWKim marked this pull request as ready for review December 22, 2024 07:20
group_by: None,
order_by: None,
aggregates: vec![],
limit: None, // TODO: add support for limit
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be fairly simple to add, search for DecrJumpZero in emitter.rs and put one of those in emit_delete_insns()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to maintain this PR small for now. And I want to improve my understanding on limit(+ order by).

What do you think of I add a follow up PR right after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my mind. Added limit support 🤖

@jussisaurio
Copy link
Collaborator

@penberg do you have any infinite wisdom about what we should do about this PR? the query planner part for DELETE looks good, but since there is no actual delete implementation, it just prints which rowids it would have deleted. should we merge this behind a default-off feature flag to reduce confusion in case someone actually uses the lib

@penberg
Copy link
Collaborator

penberg commented Dec 27, 2024

@jussisaurio @seonWKim I think we can just merge this (as soon as someone fixes the merge conflicts). We can hide it under a general-purpose feature flag like "experimental" or "preview" but I don't think that's necessarily needed right now. We're at 0.0.10 so hopefully people understand that many things just don't work.

@seonWKim
Copy link
Contributor Author

@penberg I'll resolve the conflict and let you know

@penberg penberg merged commit ae47665 into tursodatabase:main Dec 27, 2024
35 checks passed
@seonWKim seonWKim deleted the feature/delete-planning branch December 27, 2024 23:40
@seonWKim
Copy link
Contributor Author

#455

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