Repository navigation
Conversation
…mats
Compression.NONE is a truthy Enum member, so the `if compression:` guard
in CSVFormat/TSVFormat/NDJSONFormat/ParquetFormat treated the
no-compression sentinel as 'a codec was specified'. For ParquetFormat this
raised TypeError('Compression should be None, ZStd, or Snappy.') outright;
for the others it emitted a spurious COMPRESSION = NONE option.
Guard now skips both None and Compression.NONE, so the uncompressed case
renders no COMPRESSION option. Explicit codecs are unchanged, and
ParquetFormat still rejects codecs its writer can't use (gzip/bz2/zip).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9239985e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) | ||
| self.options["MISSING_FIELD_AS"] = f"{missing_field_as}" | ||
| if compression: | ||
| if compression and compression is not Compression.NONE: |
There was a problem hiding this comment.
When callers pass ParquetFormat(compression=Compression.NONE), this condition now skips validation and emits no COMPRESSION option. For Parquet, Databend only supports ZSTD/SNAPPY for this option and the omitted-option default is ZSTD, so a user asking for no compression silently gets ZSTD-compressed output instead of the previous clear rejection. Keep Compression.NONE rejected for Parquet rather than treating it like None.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c83bc03. NONE is no longer dropped; it's emitted as
COMPRESSION = NONE, which current Databend honours for Parquet (verified with
inspect_parquet).
|
CI failure is unrelated to this PR — it's a Databend nightly server flake. This PR only changes COPY INTO compression handling (dml.py) and its copy-format tests; The run was 707 passed, 677 skipped, 2 errors — the 2 errors are both at teardown of the The server both refuses to create a sequence (already exists) and then can't find it to drop it. Root cause: CI is pinned to databend:nightly. Last green main was 2026-02-28; these runs are on Suggest a re-run (may pass on retry) and/or pinning CI to a known-good Databend tag. Happy to file |
hantmac
left a comment
hantmac
left a comment
There was a problem hiding this comment.
Thanks for adding regression coverage and making the compilation-cache policy explicit. I cannot merge this as-is: None (use the server default) and Compression.NONE (explicitly disable compression) are not equivalent for Parquet. I reproduced the generated SQL locally and checked the Databend option parser/defaults. Please preserve the explicit no-compression request and update the tests rather than asserting that it disappears. The existing inline finding about the Parquet default remains unresolved.
This is a correctness issue independent of the historical CI failure; those old logs have expired, so I could not independently verify the author's sequence-flake diagnosis.
| ) | ||
| self.options["MISSING_FIELD_AS"] = f"{missing_field_as}" | ||
| if compression: | ||
| if compression and compression is not Compression.NONE: |
There was a problem hiding this comment.
[P2] Do not silently turn explicit NONE into the Parquet default.
With this change, ParquetFormat(compression=Compression.NONE) compiles to FILE_FORMAT = (TYPE = PARQUET). Databend fills in ZSTD when the option is omitted, so a request for uncompressed output silently produces compressed output. None should mean 'leave the server default', not 'uncompressed'.
Source: Parquet option parsing defaults to Zstd. Current Databend also has a Parquet NONE regression test, so NONE is not inherently an invalid SQL option on current servers.
Please distinguish omission from explicit NONE: emit COMPRESSION = NONE when supported, or keep an explicit rejection for any supported server versions that cannot honor it. Do not silently omit it. Adjust test_no_compression_emits_no_option accordingly, and preferably verify the resulting Parquet codec in an integration test. The same sentinel-erasing guards for the text formats are unnecessary: their existing COMPRESSION = NONE is a legitimate explicit option.
There was a problem hiding this comment.
You're right: omission and explicit NONE aren't the same for Parquet, and I'd
conflated them. Fixed in c83bc03:
ParquetFormat(compression=Compression.NONE)now emitsCOMPRESSION = NONE;
Nonestill omits it and gets the server's ZSTD default.- The CSV/TSV/NDJSON guards are reverted, so they emit
COMPRESSION = NONEagain,
as before. test_no_compression_emits_no_optionis split into
test_omitted_compression_emits_no_optionandtest_explicit_none_is_emitted.- Added the integration test you suggested: unload to a stage and compare
inspect_parquet's compressed vs uncompressed row-group sizes. Against v1.2.881:
NONE 247/247, ZSTD 63/246, SNAPPY 61/246, omitted 63/246.
On older servers: the Parquet writer's NONE arm arrived with
databendlabs/databend#17793. Before that, from reading the source rather than
running an old server, NONE ends in an error, so the request should still fail
loudly rather than being silently changed. That seemed better than a client-side
version check, but I can add one if you'd prefer.
Omitting COMPRESSION means the server default, and for Parquet that is ZSTD, so dropping an explicit Compression.NONE silently produced compressed output. CSV/TSV/NDJSON go back to emitting it as before, and ParquetFormat now accepts NONE alongside ZSTD and SNAPPY. The unload test checks the written file's codec via inspect_parquet: compressed and uncompressed row-group sizes match only for NONE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
simozzy
changed the title
|
Thanks for the careful review. Fixed in c83bc03, details on the inline thread. On CI: I re-checked locally against a Databend v1.2.881 release binary with |
|
@hantmac Could you possibly check why the CI runs are failing? |
Problem
ParquetFormat(compression=Compression.NONE)raisesTypeError: Compression should be None, ZStd, or Snappy., so there is no way torequest uncompressed Parquet output. Databend accepts
COMPRESSION = NONEforParquet and writes
UNCOMPRESSED; omitting the option gives the server default,which is ZSTD.
Fix
ParquetFormatacceptsCompression.NONEalongsideZSTDandSNAPPYand emitsit as
COMPRESSION = NONE.Nonestill omits the option. CSV/TSV/NDJSON areunchanged.
The four format classes also set
inherit_cache = False(matchingCopyIntoTable/CopyIntoLocation). Without it, compiling a format raisesSQLAlchemy's "will not make use of SQL compilation caching" SAWarning, which the
test suite treats as an error.
Tests
tests/test_copy_format.py:NoneomitsCOMPRESSION;Compression.NONEemitsCOMPRESSION = NONEfor all four formats; explicit codecs are emitted;ParquetFormatrejects GZIP/BZ2/ZIP.COPY INTOa stage, then read the file's row-group sizes withinspect_parquet. WithCompression.NONEthe compressed and uncompressed sizesare equal; with the option omitted, compressed is smaller.
Run against Databend v1.2.881: the unload test fails on this PR's previous
revision (
66 != 249, i.e. ZSTD was written) and onmain(theTypeError), andpasses with this change.
🤖 Generated with Claude Code