feat(pgwire):support portal description #3397

ZENOTME · 2022-06-22T06:28:58Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Support portal type in describe message. It is required when using JDBC.

PLEASE DO NOT LEAVE THIS EMPTY !!!

Please explain IN DETAIL what the changes are in this PR and why they are needed:

Summarize your change (mandatory)
How does this PR work? Need a brief introduction for the changed logic (optional)
Describe clearly one logical change and avoid lazy messages (optional)
Describe any limitations of the current code (optional)
Add the 'user-facing changes' label if your PR contains changes that are visible to users (optional)

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)

#3392

codecov · 2022-06-22T06:40:20Z

Codecov Report

Merging #3397 (001f9c0) into main (aac69d2) will decrease coverage by 0.00%.
The diff coverage is 60.93%.

@@            Coverage Diff             @@
##             main    #3397      +/-   ##
==========================================
- Coverage   73.73%   73.72%   -0.01%     
==========================================
  Files         761      761              
  Lines      104467   104506      +39     
==========================================
+ Hits        77030    77049      +19     
- Misses      27437    27457      +20

Flag	Coverage Δ
rust	`73.72% <60.93%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
src/utils/pgwire/src/pg_message.rs	`74.18% <ø> (-0.01%)`	⬇️
src/utils/pgwire/src/pg_protocol.rs	`73.77% <47.05%> (-2.61%)`	⬇️
src/utils/pgwire/src/pg_extended.rs	`77.73% <76.66%> (-0.48%)`	⬇️
src/frontend/src/expr/utils.rs	`98.99% <0.00%> (-0.26%)`	⬇️
src/common/src/types/ordered_float.rs	`24.70% <0.00%> (-0.20%)`	⬇️
src/meta/src/barrier/mod.rs	`69.59% <0.00%> (+0.29%)`	⬆️

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

BowenXiao1999 · 2022-06-22T06:34:35Z

src/utils/pgwire/src/pg_extended.rs

        let statement = cstr_to_str(&self.query_string).unwrap().to_owned();

        if params.is_empty() {
-            return PgPortal {
+            let row_description =
+                if statement.starts_with("select") || statement.starts_with("SELECT") {


I' m not sure whether query can be sElEcT

BowenXiao1999 · 2022-06-22T06:35:05Z

src/utils/pgwire/src/pg_extended.rs

+                    if let Ok(rows) = session.infer_return_type(statement.as_str()).await {
+                        rows
+                    } else {
+                        return Err(());


What about use ?

What about use ?

infer_return_type -> Result<-, BoxedError>
We can't covert Result<-, BoxedError> to Result<-,()> when using '?'. Or is there another way can process it more concisely?

try .map_err?

BowenXiao1999 · 2022-06-22T06:39:19Z

src/utils/pgwire/src/pg_extended.rs

+        let row_description = if instance_query_string.starts_with("select")
+            || instance_query_string.starts_with("SELECT")


This part of code is very similar to above.

We can extract a function inside portal?
fn infer_row_description(sql: &str,session: Arc<SM::Session>,) -> Result<Vec<PgFieldDescriptor>, ()>

BowenXiao1999 · 2022-06-22T06:40:56Z

src/utils/pgwire/src/pg_protocol.rs

+                    // TODO: Error Handle
+                    todo!()


Link a TODO issue in code. You can check #2366. Investigate whether we can improve Error in the pgwire-crates

BowenXiao1999 · 2022-06-22T06:50:50Z

src/utils/pgwire/src/pg_protocol.rs

+                let session = self.session.clone().unwrap();
+                let portal = statement
+                    .instance::<SM>(session, portal_name.clone(), &m.params)


directly self.session.clone(). No need for a temporal variable here.

BowenXiao1999 · 2022-06-22T06:54:10Z

src/utils/pgwire/src/pg_protocol.rs

-                    // NOTE Error handle need modify later.
-                    named_statements.get(&name).unwrap()
-                };
+                if m.kind == b'S' {


Add some comments explain what is 'S' or 'P' ?

neverchanje · 2022-06-22T07:11:00Z

src/utils/pgwire/src/pg_extended.rs

        let statement = cstr_to_str(&self.query_string).unwrap().to_owned();

        if params.is_empty() {
-            return PgPortal {
+            let row_description =
+                if statement.starts_with("select") || statement.starts_with("SELECT") {


I guess you should take the first 6 characters and case-insensitively compare them with "select", which means you can convert all characters to lower-case. I don't know if "Select" would work?

BowenXiao1999

Rest LGTM

BowenXiao1999 · 2022-06-22T09:17:55Z

src/utils/pgwire/src/pg_extended.rs

+        if sql.len() > 6 && sql[0..6].eq_ignore_ascii_case("select") {
+            session.infer_return_type(sql).await.map_err(|_e| ())
+        } else {
+            Ok(vec![])
+        }


if sql.len() > 6 && sql[0..6].eq_ignore_ascii_case("select") { return session.infer_return_type(sql).await.map_err(|_e| ()); } Ok(vec![]) ``` Try this?

Would it be more efficient

More clean. We should redcue the if-else pairs. One if is more readable than if-else

ZENOTME requested a review from BowenXiao1999 June 22, 2022 06:28

github-actions bot added Invalid PR Title type/feature labels Jun 22, 2022

ZENOTME changed the title ~~feature(pgwire):support portal description~~ feat(pgwire):support portal description Jun 22, 2022

github-actions bot removed the Invalid PR Title label Jun 22, 2022

BowenXiao1999 reviewed Jun 22, 2022

View reviewed changes

neverchanje reviewed Jun 22, 2022

View reviewed changes

ZENOTME added 5 commits June 22, 2022 16:56

add row_description in portal

adf44de

add process portal description

d27c074

add tracing in execute

0f6feb7

return row_description only when meet a DQL(SELECT)

cda6297

fix clippy

a710f17

ZENOTME force-pushed the zj/portal_describe branch from eba89d8 to 5d2352d Compare June 22, 2022 09:01

ZENOTME requested review from BowenXiao1999 and neverchanje June 22, 2022 09:17

BowenXiao1999 approved these changes Jun 22, 2022

View reviewed changes

fix

4759a67

ZENOTME force-pushed the zj/portal_describe branch from 5d2352d to 4759a67 Compare June 22, 2022 09:26

BowenXiao1999 linked an issue Jun 22, 2022 that may be closed by this pull request

feature(pgwire):support portal description #3392

Closed

ZENOTME added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jun 22, 2022

Merge branch 'main' into zj/portal_describe

001f9c0

mergify bot merged commit 86e7742 into main Jun 22, 2022

mergify bot deleted the zj/portal_describe branch June 22, 2022 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pgwire):support portal description #3397

feat(pgwire):support portal description #3397

ZENOTME commented Jun 22, 2022 •

edited

codecov bot commented Jun 22, 2022 •

edited

BowenXiao1999 Jun 22, 2022

BowenXiao1999 Jun 22, 2022

ZENOTME Jun 22, 2022

BowenXiao1999 Jun 22, 2022

BowenXiao1999 Jun 22, 2022

ZENOTME Jun 22, 2022

BowenXiao1999 Jun 22, 2022

BowenXiao1999 Jun 22, 2022

BowenXiao1999 Jun 22, 2022

neverchanje Jun 22, 2022

BowenXiao1999 left a comment

BowenXiao1999 Jun 22, 2022

ZENOTME Jun 22, 2022

BowenXiao1999 Jun 22, 2022

		let row_description = if instance_query_string.starts_with("select")
		\|\| instance_query_string.starts_with("SELECT")

feat(pgwire):support portal description #3397

feat(pgwire):support portal description #3397

Conversation

ZENOTME commented Jun 22, 2022 • edited

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Jun 22, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BowenXiao1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZENOTME commented Jun 22, 2022 •

edited

codecov bot commented Jun 22, 2022 •

edited