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(pgwire): add infer_return_type interface in session trait #3161

Merged
merged 7 commits into from
Jun 15, 2022

Conversation

ZENOTME
Copy link
Contributor

@ZENOTME ZENOTME commented Jun 13, 2022

What's changed and what's your intention?

  • Add the infer_return_type interface in the session trait.
  • Use the infer_return_type interface in pgwire to get the row_description.
  • To test the modification of pgwire, add the implementation of infer_return_type in mock_session and pass the original unit test.
  • add simple infer_return_type implementation in frontend session to support no param SQL.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

#3131

@ZENOTME ZENOTME changed the title Zj/add pgwire infer return type feat(pgwire): add infer_return_type interface in session trait Jun 13, 2022
Comment on lines 176 to 198
let rows: Vec<PgFieldDescriptor> = query
.split(&[' ', ',', ';'])
.skip(1)
.into_iter()
.map(|x| {
if let Some(str) = x.strip_prefix('$') {
if let Ok(i) = str.parse() {
i
} else {
-1
}
} else {
-1
}
})
.take_while(|x: &i32| x.is_positive())
.map(|x| {
// NOTE Make sure the type_description include all generic parametre
// description we needed.
assert!((x.sub(1) as usize) < types.len());
PgFieldDescriptor::new(String::new(), types[x.sub(1) as usize].to_owned())
})
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these part of logic looks so complicated while the impl in Session is so short...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the original implement needs to infer the row description from the types descriptions. Now, We just simply get the row description from the infer_return_type. The complicated parts are left to the implementation of infer_return_type.

Copy link
Contributor

@BowenXiao1999 BowenXiao1999 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #3161 (486945e) into main (6e7ca58) will decrease coverage by 0.00%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
- Coverage   73.50%   73.49%   -0.01%     
==========================================
  Files         746      746              
  Lines      101925   101981      +56     
==========================================
+ Hits        74916    74951      +35     
- Misses      27009    27030      +21     
Flag Coverage Δ
rust 73.49% <70.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/frontend/src/session.rs 43.22% <0.00%> (-2.26%) ⬇️
src/utils/pgwire/src/pg_protocol.rs 75.71% <85.29%> (-0.45%) ⬇️
src/utils/pgwire/src/pg_extended.rs 90.10% <100.00%> (+2.97%) ⬆️
src/utils/pgwire/src/pg_server.rs 89.42% <100.00%> (+0.82%) ⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs 42.85% <0.00%> (-1.10%) ⬇️
src/storage/src/hummock/local_version_manager.rs 83.86% <0.00%> (-0.16%) ⬇️

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

@ZENOTME ZENOTME force-pushed the zj/add_pgwire_infer_return_type branch from 5bf3193 to 486945e Compare June 15, 2022 02:54
@ZENOTME ZENOTME merged commit 0f73147 into main Jun 15, 2022
@ZENOTME ZENOTME deleted the zj/add_pgwire_infer_return_type branch June 15, 2022 04:31
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.

None yet

2 participants