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: Convert GRPC results to Row #812

Merged
merged 9 commits into from
Mar 11, 2022
Merged

feat: Convert GRPC results to Row #812

merged 9 commits into from
Mar 11, 2022

Conversation

cykbls01
Copy link
Contributor

@cykbls01 cykbls01 commented Mar 10, 2022

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

convert data_chunk to pg_response row

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)

#810

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #812 (fa31d18) into main (239bb18) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #812      +/-   ##
============================================
+ Coverage     72.01%   72.02%   +0.01%     
  Complexity     2766     2766              
============================================
  Files           925      926       +1     
  Lines         53935    53965      +30     
  Branches       1787     1787              
============================================
+ Hits          38839    38868      +29     
- Misses        14206    14207       +1     
  Partials        890      890              
Flag Coverage Δ
java 61.22% <ø> (ø)
rust 76.48% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
rust/common/src/array/macros.rs 100.00% <ø> (ø)
rust/frontend/src/handler/mod.rs 90.90% <ø> (ø)
rust/common/src/array/data_chunk.rs 94.23% <100.00%> (ø)
rust/frontend/src/handler/util.rs 100.00% <100.00%> (ø)
rust/meta/src/hummock/compaction.rs 81.65% <0.00%> (-0.60%) ⬇️

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

@fuyufjh
Copy link
Member

fuyufjh commented Mar 10, 2022

I think this should be placed in frontend because only the frontend needs it (or more precisely, only the PG protocol layer needs it)

@neverchanje
Copy link
Contributor

I think this should be placed in frontend because only the frontend needs it (or more precisely, only the PG protocol layer needs it)

Yes I used to thought the DataChunk::Row is the PgRow.

It truly should be in frontend, frontend/src/handler/util.rs.

Comment on lines 560 to 564
if let Some(t) = s {
Some(t.to_string())
} else {
None
}
Copy link
Contributor

Choose a reason for hiding this comment

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

s.map(|t| t.to_string())

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be placed in frontend because only the frontend needs it (or more precisely, only the PG protocol layer needs it)

+1

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.

Put it in utils in this PR?

Rest LGTM

@BowenXiao1999
Copy link
Contributor

I think this should be placed in frontend because only the frontend needs it (or more precisely, only the PG protocol layer needs it)

Yes I used to thought the DataChunk::Row is the PgRow.

It truly should be in frontend, frontend/src/handler/util.rs.

Previously I indeed re-use DataChunk's row, however, to publish this as a library or crate, we can not use a struct in another non-open source project. I guess that's why @wangrunji0408 introduce a new simplified Row in pgwire.

@cykbls01 cykbls01 merged commit 49b46b5 into main Mar 11, 2022
@cykbls01 cykbls01 deleted the feat/datachunk_to_row branch March 11, 2022 01:38
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