-
Notifications
You must be signed in to change notification settings - Fork 453
feat: implement postgres copy to stdout #7709
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @sunng87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature by implementing the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements COPY (query) TO STDOUT for the PostgreSQL interface, which is a required optimization for ADBC-postgres. The approach of handling this at the protocol layer while treating it as a standard query in the engine is a good design choice. The code is generally well-structured, with good refactoring to support different encoders.
I've identified a critical issue in how multi-statement queries are handled, which could lead to incorrect output formatting. There's also an issue with the format code used in the CopyOutResponse, which violates the PostgreSQL protocol. Additionally, I've noted a medium-severity concern regarding the use of forked git dependencies, which could impact long-term maintainability. None of the comments contradict the provided rule regarding vector index searches and query optimization.
| pgwire = { git = "https://github.com/sunng87/pgwire.git", branch = "feature/copy-encoding" } | ||
| arrow-pg = { git = "https://github.com/datafusion-contrib/datafusion-postgres.git", branch = "feature/arrow-pg-copy-encoder"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using git dependencies with specific branches can be risky for long-term maintainability and build reproducibility. Could you please add a note or link to the upstream PRs for these changes? It's important to ensure these changes are merged upstream and the dependencies are updated to released versions from crates.io before this feature is considered stable.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#7706
What's changed and what's your intention?
This patch implements
COPY (query) TO STDOUT WITH (format binary), which is required by ADBC-postgres for optimizations.PR Checklist
Please convert it to a draft if some of the following conditions are not met.