Skip to content

refactor: Use String to replace Expression in DeletePlan#7716

Merged
bohutang merged 3 commits intodatabendlabs:mainfrom
Xuanwo:delete-v2
Sep 19, 2022
Merged

refactor: Use String to replace Expression in DeletePlan#7716
bohutang merged 3 commits intodatabendlabs:mainfrom
Xuanwo:delete-v2

Conversation

@Xuanwo
Copy link
Copy Markdown
Member

@Xuanwo Xuanwo commented Sep 19, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Related to #7593

/// # TODO
///
/// From @xuanwo
///
/// Ideally, we need to use `Scalar` in DeletePlan.selection. But we met a
/// cycle deps here. So we have to change `selection` in String first, and
/// change into `Scalar` when our `Planner` has been moved out.
///
/// At this stage, DeletePlan's selection expr will be parsed twice:
///
/// - Parsed during `bind` to get column index and projection index.
/// - Parsed during `execution` to get the correct columns
///
/// It's an ugly but necessary price to pay. Without this, we would sink in
/// hell forever.

With this PR, bind_delete will not have external deps anymore.

I know this is not the best code we can write. But there is no other choice...

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@vercel
Copy link
Copy Markdown

vercel bot commented Sep 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Sep 19, 2022 at 6:07AM (UTC)

@mergify mergify bot added the pr-refactor this PR changes the code base without new features or bugfix label Sep 19, 2022
Signed-off-by: Xuanwo <github@xuanwo.io>
@bohutang bohutang merged commit c6d644d into databendlabs:main Sep 19, 2022
@Xuanwo Xuanwo deleted the delete-v2 branch September 21, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-refactor this PR changes the code base without new features or bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants