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

[SNOW-1649438] add zstd decompressor in the urllib3 response #2043

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sfc-gh-dbouassida
Copy link

@sfc-gh-dbouassida sfc-gh-dbouassida commented Sep 3, 2024

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    SNOW-1649438

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

This PR adds support for decompressing zstd (Zstandard) compressed responses in the urllib3 response handler. Key changes:

  • Import zstandard library and check version (requires 0.18.0+)
  • Add ZstdDecoder class to handle zstd decompression
  • Add "zstd" to list of supported content decoders
  • Update decoder initialization and error handling to include zstd

This enhancement allows the Snowflake connector to properly handle and decompress zstd-compressed responses from the Snowflake service, to improve compatibility and performance.

Resolves: SNOW-1649438

Copy link

github-actions bot commented Sep 3, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sfc-gh-dbouassida sfc-gh-dbouassida changed the title add zstd decompressor in the urllib3 response [SNOW-1649438] add zstd decompressor in the urllib3 response Sep 3, 2024
@sfc-gh-dbouassida sfc-gh-dbouassida marked this pull request as ready for review September 3, 2024 19:36
@sfc-gh-dbouassida
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@sfc-gh-mkeller
Copy link
Collaborator

recheck

@sfc-gh-mkeller
Copy link
Collaborator

The PR looks good by me, how do you plan on testing your feature @sfc-gh-dbouassida ? Can you add some unit tests, or even some integration test? Or is manual testing the best we can do and if so please describe how you have tested this!

@sfc-gh-aling can you guys chime in, please? Dhia's internship is ending soon!

@@ -17,6 +17,23 @@
except ImportError:
brotli = None

try:
import zstandard as zstd
Copy link
Collaborator

@sfc-gh-aling sfc-gh-aling Sep 11, 2024

Choose a reason for hiding this comment

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

I'm interested when the backend sends back zstd compressed data, is there a configuration?

also what error would connector trigger if it receives zstd compressed data but zstd is not installed in the execution env

@@ -126,6 +143,29 @@ def flush(self):
return b""


if HAS_ZSTD:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious when is this zstd feature introduced in the urllib?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants