Skip to content

refactor: Old Planner Never See Again (Part X) #7593

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

Closed
wants to merge 12 commits into from

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Sep 13, 2022

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

Summary

This PR is trying to remove the ExprParser.

After this PR, our storage will don't depend on sqlparser anymore.

This PR introduces a dangerous function, as I said in comment:

/// unchecked_expressions_analyze will analyzer given expr str into `Expression`.
///
/// `unchecked` means there is no correction checks for expr:
///
/// - We don't if the expr is **safe**.
/// - We don't if the expr is **existed**.
/// - We don't if the expr is **valid**.
///
/// We just convert the given expr into Expression, and hoping everything works.
///
/// This function should only be used by very limited use-cases like fuse engines
/// `cluster_keys` which already checked by our parsers.
///
/// **FBI WARNING: DON'T USE THIS FUNCTION UNLESS YOU KNOW WHAT I MEAN**
///
/// Any usage to this function should be reviewed carefully.
///
/// # Future Plan
///
/// This function should be removed once our new expression framework is ready.
pub fn unchecked_expressions_analyze(expr: &str) -> Result<Vec<Expression>> { ... }

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

vercel bot commented Sep 13, 2022

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

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Sep 17, 2022 at 1:39PM (UTC)

Signed-off-by: Xuanwo <github@xuanwo.io>
@mergify mergify bot added the pr-refactor this PR changes the code base without new features or bugfix label Sep 13, 2022
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as draft September 13, 2022 13:48
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 13, 2022

unchecked_expressions_analyze can't parse complex cluster key, I'm seeking new ways to address this problem.

@Xuanwo Xuanwo self-assigned this Sep 15, 2022
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 17, 2022

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Sep 17, 2022

update

✅ Branch has been successfully updated

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 17, 2022

This PR is blocked by Planner.

Now, planner is implemented inside query/service/src/sql/planner, and It depends on types like crate::evaluator::Evaluator to parse DataBlock at bind time (aka, ValueSourceV2). Thus, we can't move Planner out to be a standalone crate common-planner.

However, common-storages-fuse needs Planner to parse exprs correctly (and safely).

Ideally, we should not parse DataBlock during bind. Can we move that logic to interpreter-insert?

Cc @leiysky @sundy-li

@sundy-li
Copy link
Member

Ideally, we should not parse DataBlock during bind. Can we move that logic to interpreter-insert?

Yes, I think it'll be done in #7613

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 17, 2022

Yes, I think it'll be done in #7613

Cc @youngsofun for confirmation

@sundy-li
Copy link
Member

I'll try to apply a fast hot pr to improve this.

@Xuanwo Xuanwo changed the title refactor: Old Planner Never See Again (Part 2) refactor: Old Planner Never See Again (Part X) Sep 21, 2022
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 21, 2022

This PR is out-of-date, I will start a new to replace it.

@Xuanwo Xuanwo closed this Sep 21, 2022
@Xuanwo Xuanwo deleted the never-see-again branch September 21, 2022 16:49
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