kaxil commented on code in PR #70302:
URL: https://github.com/apache/airflow/pull/70302#discussion_r3770271074
##########
providers/common/ai/src/airflow/providers/common/ai/utils/file_analysis.py:
##########
@@ -536,10 +552,10 @@ def _render_avro(path: ObjectStoragePath, *, sample_rows:
int, max_content_bytes
def _read_raw_bytes(path: ObjectStoragePath, *, compression: str | None,
max_bytes: int) -> bytes:
with path.open("rb") as handle:
- if compression == "gzip":
- with gzip.GzipFile(fileobj=handle) as gzip_handle:
- return _read_limited_bytes(gzip_handle, path=path,
max_bytes=max_bytes)
- return _read_limited_bytes(handle, path=path, max_bytes=max_bytes)
+ if compression is None:
+ return _read_limited_bytes(handle, path=path, max_bytes=max_bytes)
+ with _DECOMPRESSORS[compression](handle) as decompressed:
Review Comment:
One asymmetry inside this dispatch: `gzip.open` raises `BadGzipFile` if
anything follows a complete stream, but `bz2.open` and `lzma.open` silently
return just the first stream, so `_read_limited_bytes` sees a normal EOF and
nothing marks the content as incomplete. The spec-legal case is the interesting
one: with 4 null bytes of xz Stream Padding between two streams, `xz -t` passes
and `xz -dc` prints both, while `lzma.open` returns only the first. Pre-PR
these inputs were rejected outright, so it's worth either treating non-null
`unused_data` after `eof` as an error via `BZ2Decompressor`/`LZMADecompressor`,
or pinning the current behavior in a test and noting the limitation in the docs.
##########
providers/common/ai/tests/unit/common/ai/utils/test_file_analysis.py:
##########
@@ -378,19 +383,43 @@ def
test_detect_file_format_rejects_unsupported_compression(self, tmp_path):
with pytest.raises(LLMFileAnalysisUnsupportedFormatError,
match="Compression"):
detect_file_format(ObjectStoragePath(str(path)))
- @pytest.mark.parametrize("filename", ["sample.parquet.gz",
"sample.avro.gz", "sample.png.gz"])
- def
test_detect_file_format_rejects_unsupported_gzip_format_combinations(self,
tmp_path, filename):
+ @pytest.mark.parametrize(("filename", "codec"), [("events.csv.bz2",
"bzip2"), ("events.json.xz", "xz")])
+ def test_detect_file_format_rejects_compression_without_codec_module(self,
tmp_path, filename, codec):
path = tmp_path / filename
path.write_bytes(b"content")
- with pytest.raises(LLMFileAnalysisUnsupportedFormatError, match="not
supported for"):
+ with patch.dict(_DECOMPRESSORS, {"gzip": gzip.open}, clear=True):
Review Comment:
This pins the rejection but not the half I was actually worried about, that
plain and gzip inputs keep working while the codecs are missing, which is one
more assert inside the same patched block. Related, `import bz2` and `import
lzma` at the top of this module are unguarded, so on exactly those builds the
module fails to collect, and `sample.avro.bz2` / `sample.png.xz` in the
combination test would hit the codec-level message and fail its regex rather
than skip. `pytest.importorskip` (already used for pyarrow and fastavro below)
plus `.gz` params for the combination test covers it, and the expansion-bound
tests at 262 and 279 are still gzip-only if you want to reuse the parametrize
you added at 410.
##########
providers/common/ai/src/airflow/providers/common/ai/utils/file_analysis.py:
##########
@@ -370,12 +386,12 @@ def detect_file_format(path: ObjectStoragePath) ->
tuple[str, str | None]:
raise LLMFileAnalysisUnsupportedFormatError(
f"Unsupported file format {detected!r} for {path}. Supported
formats: {', '.join(SUPPORTED_FILE_FORMATS)}."
)
- if compression and compression != "gzip":
+ if compression and compression not in _DECOMPRESSORS:
Review Comment:
On a build without `_lzma` this branch fires first and reports `Compression
'xz' is not supported for file analysis.`, the same message zstd gets, even
though the docs now say xz is supported, so it points people at their file
rather than at their Python build. It also makes the message for something like
`sample.parquet.bz2` depend on the build, since the format-combination check
below only runs when the codec module is present. Checking a static `{"gzip",
"bzip2", "xz"}` set here and raising `AirflowOptionalProviderFeatureException`
for a known-but-missing codec would match what `_render_parquet` and
`_render_avro` already do lower down, and the rst could gain a clause saying
bzip2/xz need an interpreter built with those modules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
