Skip to content

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>> { ... }

@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 <[email protected]>
@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 <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@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