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

feat(frontend): get pg_field_descs from boudstatement #950

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Conversation

cykbls01
Copy link
Contributor

@cykbls01 cykbls01 commented Mar 16, 2022

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

get pg_field_descs from boudstatement to build pg_response

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #950 (2192036) into main (38fe36a) will increase coverage by 0.01%.
The diff coverage is 63.46%.

@@             Coverage Diff              @@
##               main     #950      +/-   ##
============================================
+ Coverage     71.52%   71.54%   +0.01%     
  Complexity     2766     2766              
============================================
  Files           973      973              
  Lines         57427    57473      +46     
  Branches       1790     1790              
============================================
+ Hits          41076    41119      +43     
- Misses        15460    15463       +3     
  Partials        891      891              
Flag Coverage Δ
java 61.03% <ø> (ø)
rust 75.55% <63.46%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/frontend/src/binder/delete.rs 100.00% <ø> (ø)
rust/frontend/src/binder/insert.rs 100.00% <ø> (ø)
rust/frontend/src/binder/query.rs 100.00% <ø> (ø)
rust/frontend/src/binder/select.rs 98.07% <ø> (ø)
rust/frontend/src/binder/set_expr.rs 80.00% <ø> (ø)
rust/frontend/src/binder/statement.rs 87.50% <ø> (ø)
rust/frontend/src/binder/table_ref.rs 77.33% <ø> (ø)
rust/frontend/src/binder/values.rs 91.11% <ø> (ø)
rust/frontend/src/handler/query.rs 17.50% <0.00%> (+1.59%) ⬆️
rust/frontend/src/handler/util.rs 78.75% <66.00%> (-21.25%) ⬇️
... and 2 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@cykbls01 cykbls01 marked this pull request as ready for review March 16, 2022 12:08
@cykbls01 cykbls01 merged commit 1864737 into main Mar 16, 2022
@cykbls01 cykbls01 deleted the feat/query_desc branch March 16, 2022 13:57
@@ -27,7 +25,7 @@ pub async fn handle_query(context: QueryContext, query: Box<Query>) -> Result<Pg
let mut binder = Binder::new(catalog);
let bound = binder.bind(Statement::Query(query))?;
let plan = Planner::new(Rc::new(RefCell::new(context)))
.plan(bound)?
.plan(bound.clone())?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to pass a ref to bound statement into the planner?

If we have an insert with thousands of values, I suspect this could be slow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I think it's better to take &BoundStatement for pg_field_descs. The planner needs ownership to reduce clones.

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Mar 17, 2022

Seems the logic of extracting name & type is redundant. Is it possible to just use plan.schema()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants