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: Support printing postgresql's bytea data type in its "hex" and "escape" format #3567

Merged
merged 19 commits into from Mar 27, 2024

Conversation

J0HN50N133
Copy link
Contributor

@J0HN50N133 J0HN50N133 commented Mar 22, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #3438

What's changed and what's your intention?

Support printing postgresql's bytea data type in its "hex" and "escape" format.

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

  • Summarize your change
    Support printing postgresql's bytea data type in its "hex" and "escape" format. And add a general way to set session config value, which make it easier to support Support outputting various date styles for postgresql #3442 .
  • How does this PR work? Need a brief introduction for the changed logic
    Add a new field configuration_variables in Session and QueryContext. The values in QueryContext may be changed by set statement and will be finally pass to Session. And the config value will affect the output.
    Different output of bytea is implemented by wrapper: HexOutputBytea and EscapeOutputBytea which reimplement ToSqlText and delegate ToSql to inner data.
  • Describe any limitations of the current code
    SessionConfigValue is coupled with sql::ast::Value. And configuration parameters is not well-organized.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added Invalid PR Title docs-not-required This change does not impact docs. labels Mar 22, 2024
@J0HN50N133
Copy link
Contributor Author

@MichaelScofield I would like to add some related integration test, but didn't find a way. Would you please tell me how to do this?

@J0HN50N133 J0HN50N133 changed the title Support printing postgresql's bytea data type in its "hex" and "escape" format feat: Support printing postgresql's bytea data type in its "hex" and "escape" format Mar 22, 2024
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 76.68161% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 84.79%. Comparing base (58c7858) to head (6ae783f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3567      +/-   ##
==========================================
- Coverage   85.20%   84.79%   -0.42%     
==========================================
  Files         926      928       +2     
  Lines      153645   153851     +206     
==========================================
- Hits       130919   130461     -458     
- Misses      22726    23390     +664     

src/session/src/context.rs Outdated Show resolved Hide resolved
src/servers/src/postgres/handler.rs Outdated Show resolved Hide resolved
src/servers/src/postgres/types/bytea.rs Show resolved Hide resolved
src/session/src/lib.rs Outdated Show resolved Hide resolved
tests-integration/tests/sql.rs Outdated Show resolved Hide resolved
@MichaelScofield
Copy link
Collaborator

@MichaelScofield I would like to add some related integration test, but didn't find a way. Would you please tell me how to do this?

I'd suggest putting the tests in file "/tests-integration/tests/sql.rs". But I see you've already done this.

src/operator/src/statement.rs Outdated Show resolved Hide resolved
src/operator/src/error.rs Show resolved Hide resolved
src/session/src/context.rs Outdated Show resolved Hide resolved
src/session/src/session_config.rs Outdated Show resolved Hide resolved
src/session/src/context.rs Outdated Show resolved Hide resolved
refactor: use Arc<DashMap> instead of DashMap in QueryContext

refactor: use Arc<DashMap> instead of DashMap in QueryContext

    Avoid expensive clone

refactor: use unreachable!() instead of unimplemented!()

refactor: move the encode bytea by format type logic into encoder

test: add binary format integration test case
@J0HN50N133
Copy link
Contributor Author

@sunng87 Thanks for your advice, helps simplifying the implementation a lot.

Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @J0HN50N133 for your patient and fast response!

@MichaelScofield MichaelScofield added this pull request to the merge queue Mar 27, 2024
Merged via the queue into GreptimeTeam:main with commit 83643eb Mar 27, 2024
18 checks passed
@J0HN50N133 J0HN50N133 deleted the feature_bytea branch March 27, 2024 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support printing postgresql's bytea data type in its "hex" and "escape" format
4 participants