You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/07/19 04:04:30 UTC
[PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
aglinxinyuan opened a new pull request, #6545:
URL: https://github.com/apache/texera/pull/6545
### What changes were proposed in this PR?
Follow-up to #6488, which made Iceberg local storage work on Windows without
winutils **on the Scala/JVM side only** (`IcebergUtil` + `WinutilsFreeLocalFileSystem`).
The Python UDF worker writes its output-port Iceberg storage through its **own**
pyiceberg path — `core/storage/iceberg/iceberg_document.py` and `document_factory.py`
call `IcebergCatalogInstance.get_instance()` → `create_postgres_catalog` inside the
Python process — so #6488 does not reach it. On a Windows dev machine with a
local-filesystem warehouse (`postgres` catalog type), that path fails two ways:
| # | Symptom | Root cause |
|---|---|---|
| 1 | `ValueError: Unrecognized filesystem type in URI: c` at table creation | a bare drive path `C:\...` is parsed by pyiceberg as URI scheme `c` |
| 2 | `OSError: [WinError 123] ... The filename, directory name, or volume label syntax is incorrect` | even a `file:///C:/...` URI is rejected by pyiceberg's default **pyarrow** FileIO (`/C:/...`) |
Fix (in `iceberg_utils.py`, mirroring #6488's tight, self-gating approach):
| Change | Detail |
|---|---|
| `_is_windows_local_warehouse(warehouse)` (new) | True only when the warehouse carries a **Windows drive letter** — bare (`C:\...`, `C:/...`) or `file:///C:/...`. Excludes POSIX paths, colon-stripped paths (`C/...`), and remote object stores (`s3://...`). |
| `_to_file_uri(warehouse)` (new) | Normalizes a bare drive path to a `file:///` URI via `PureWindowsPath.as_uri()` (deterministic on every host OS); already-URI values pass through. |
| `create_postgres_catalog` | When (and only when) the warehouse is Windows-local, register the normalized `file:///` URI and select `pyiceberg.io.fsspec.FsspecFileIO`, whose `LocalFileSystem` handles Windows drive paths. |
| `create_rest_catalog` | Unchanged — its `warehouse_name` is a logical identifier resolved server-side via `S3FileIO`, not a local path (checked, exempt). |
The gate is deliberately narrow so the fix cannot alter the cross-runtime
warehouse convention that Linux, macOS, CI, and the Scala engine rely on: PyIceberg
persists the `warehouse` value into table metadata, so a `postgres` catalog with a
POSIX or remote warehouse must keep the plain-path / default-FileIO behavior
(regression fixed in #4409). Only genuine Windows drive-letter warehouses are
normalized.
The drive path is normalized with `PureWindowsPath.as_posix()` (not `.as_uri()`)
so a warehouse containing a space — e.g. `C:\Users\John Doe\...`, `C:\Program
Files\...`, OneDrive-redirected profiles — keeps a **raw** space rather than
`%20`; fsspec's `LocalFileSystem` does not URL-decode, so a `%20` would write
into a literally `%20`-named directory.
Scope note: on Windows the two runtimes register different warehouse strings into
the shared `postgres` catalog — the Scala side its existing colon-stripped path
(`C/Users/...`), the Python worker the absolute `file:///C:/...` URI. This is fine
for the actual data flow here (a Python UDF's output-port table is written by the
Python worker and read back by the engine/result service via the absolute metadata
pointer, verified end-to-end); reconciling the Scala side's colon-stripped
convention is out of scope for this fix and would belong with #6488's follow-ups.
Before → after:
| Environment (`postgres` catalog) | Before | After |
|---|---|---|
| Windows, local warehouse (`C:\...`) | worker crashes at first table create | works |
| Linux / macOS / CI, local warehouse (`/tmp/...`) | works | unchanged (gate off) |
| Any host, remote warehouse (`s3://...`) | works | unchanged (gate off) |
| `rest` catalog | unaffected (`S3FileIO`) | unaffected |
### Any related issues, documentation, discussions?
Follow-up to #6488 (Scala/JVM side); closes the Python-worker half of #6487.
Preserves the cross-runtime warehouse invariant from #4409.
### How was this PR tested?
- New `TestCreatePostgresCatalogWindowsLocal` in `test_iceberg_utils_catalog.py`
(written first, TDD): a Windows drive-letter warehouse is normalized to a
`file:///` URI and `FsspecFileIO` is selected; POSIX, colon-stripped, and
remote (`s3://`) warehouses are left untouched (no FileIO override). The tests
assert on the computed `SqlCatalog` props and use `PureWindowsPath`, so they are
OS-independent and pass on Linux CI.
- The pre-existing `TestCreatePostgresCatalog` tests (the #4409 plain-path
invariant) still pass unchanged.
- Verified end-to-end on a real Windows filesystem: a bare `C:\...` warehouse
fed through the productionized helpers into a real catalog creates a table and
round-trips rows (both failure modes above reproduced on stock `main` first).
- `amber/` `ruff check` and `ruff format --check` pass on the changed files.
(The `test_iceberg_document.py` / REST integration tests require a live
postgres / REST catalog server and are environment-gated locally; they are
unaffected by this change — confirmed identical pass/fail with the diff stashed.)
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 [1M context])
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6545:
URL: https://github.com/apache/texera/pull/6545#issuecomment-5014185164
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/6545?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.83333%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 70.94%. Comparing base ([`29656a4`](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`daa7136`](https://app.codecov.io/gh/apache/texera/commit/daa71367b5c6fa35e9792fdc21b4c34cae34430f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 15 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/texera/pull/6545?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../main/python/core/storage/iceberg/iceberg\_utils.py](https://app.codecov.io/gh/apache/texera/pull/6545?src=pr&el=tree&filepath=amber%2Fsrc%2Fmain%2Fpython%2Fcore%2Fstorage%2Ficeberg%2Ficeberg_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1iZXIvc3JjL21haW4vcHl0aG9uL2NvcmUvc3RvcmFnZS9pY2ViZXJnL2ljZWJlcmdfdXRpbHMucHk=) | 95.83% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/texera/pull/6545?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #6545 +/- ##
============================================
+ Coverage 70.93% 70.94% +0.01%
Complexity 3439 3439
============================================
Files 1145 1145
Lines 45105 45128 +23
Branches 4975 4975
============================================
+ Hits 31994 32018 +24
+ Misses 11463 11462 -1
Partials 1648 1648
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.76% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.82% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `17.72% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.66% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.80% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.07% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.94% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.76% <95.83%> (+0.06%)` | :arrow_up: | |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/6545/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.14% <ø> (ø)` | | Carriedforward from [29656a4](https://app.codecov.io/gh/apache/texera/commit/29656a4eff96671126fa9a47ec3be068b25dae43?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/6545?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #6545:
URL: https://github.com/apache/texera/pull/6545#discussion_r3609887696
##########
amber/src/main/python/core/storage/iceberg/iceberg_utils.py:
##########
@@ -35,6 +39,58 @@
# Suffix used to encode LARGE_BINARY fields in Iceberg (must match Scala IcebergUtil)
LARGE_BINARY_FIELD_SUFFIX = "__texera_large_binary_ptr"
+# pyiceberg FileIO whose fsspec LocalFileSystem handles Windows drive paths,
+# which the default pyarrow FileIO cannot. See `_is_windows_local_warehouse`.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
+# Matches a filesystem path that starts with a Windows drive letter and a
+# separator, e.g. `C:\...` or `C:/...` (but not the drive-relative `C:foo`).
+_WINDOWS_DRIVE_PATH = re.compile(r"^[A-Za-z]:[\\/]")
+
+
+def _is_windows_local_warehouse(warehouse: str) -> bool:
+ """
+ Whether ``warehouse`` is a LOCAL filesystem path carrying a Windows drive
+ letter, either as a bare path (``C:\\...`` / ``C:/...``) or a ``file://``
+ URI (``file:///C:/...``).
+
+ Remote object stores (``s3://...``), POSIX paths, drive-less relative paths,
+ and colon-stripped paths (the form the Scala side registers, ``C/...``) are
+ excluded, so the default pyarrow FileIO / plain-path convention that Linux,
+ macOS, CI, and the Scala engine rely on is left untouched (see #4409).
+ """
+ if not warehouse:
+ return False
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return True
+ parsed = urlparse(text)
+ if parsed.scheme.lower() == "file": # `file` URI schemes are case-insensitive
+ return bool(_WINDOWS_DRIVE_PATH.match(parsed.path.lstrip("/")))
+ return False
+
+
+def _to_file_uri(warehouse: str) -> str:
+ """
+ Normalize a bare Windows drive path (``C:\\...``) to a ``file:///`` URI so
+ pyiceberg does not mis-parse the drive letter as a URI scheme (``c://`` ->
+ "Unrecognized filesystem type in URI: c"). Values that are already URIs
+ (e.g. ``file:///C:/...``) are returned unchanged.
+
+ The URI is built from ``PureWindowsPath.as_posix()`` rather than
+ ``.as_uri()`` so the path is *not* percent-encoded: the fsspec
+ ``LocalFileSystem`` behind ``FsspecFileIO`` does not URL-decode, so an
+ encoded space (``C:\\Users\\John Doe`` -> ``.../John%20Doe``) would send
+ writes to a literally ``%20``-named directory. ``PureWindowsPath`` keeps the
+ conversion deterministic on every host OS (the result is identical off
+ Windows).
+ """
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return "file:///" + PureWindowsPath(text).as_posix()
Review Comment:
Good catch — refreshed the description so the fix table reads `PureWindowsPath.as_posix()`, matching the code and the paragraph below it. Thanks for the review!
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #6545:
URL: https://github.com/apache/texera/pull/6545#discussion_r3609820909
##########
amber/src/test/python/core/storage/iceberg/test_iceberg_utils_catalog.py:
##########
@@ -96,3 +100,141 @@ def test_postgres_uri_is_built_with_pg8000_scheme(self):
)
# And warehouse is still the plain path.
assert kwargs["warehouse"] == warehouse_path
+
+
+# FileIO selected for Windows-local warehouses in place of the default pyarrow
+# FileIO; its fsspec LocalFileSystem parses Windows drive paths.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
Review Comment:
Done in 900f2eb — the tests now reference the module's `_FSSPEC_FILE_IO` constant instead of the literal, so they can't drift. Thanks!
##########
amber/src/main/python/core/storage/iceberg/iceberg_utils.py:
##########
@@ -35,6 +39,57 @@
# Suffix used to encode LARGE_BINARY fields in Iceberg (must match Scala IcebergUtil)
LARGE_BINARY_FIELD_SUFFIX = "__texera_large_binary_ptr"
+# pyiceberg FileIO whose fsspec LocalFileSystem handles Windows drive paths,
+# which the default pyarrow FileIO cannot. See `_is_windows_local_warehouse`.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
+# Matches a filesystem path that starts with a Windows drive letter and a
+# separator, e.g. `C:\...` or `C:/...` (but not the drive-relative `C:foo`).
+_WINDOWS_DRIVE_PATH = re.compile(r"^[A-Za-z]:[\\/]")
+
+
+def _is_windows_local_warehouse(warehouse: str) -> bool:
+ """
+ Whether ``warehouse`` is a LOCAL filesystem path carrying a Windows drive
+ letter, either as a bare path (``C:\\...`` / ``C:/...``) or a ``file://``
+ URI (``file:///C:/...``).
+
+ Remote object stores (``s3://...``), POSIX paths, drive-less relative paths,
+ and colon-stripped paths (the form the Scala side registers, ``C/...``) are
+ excluded, so the default pyarrow FileIO / plain-path convention that Linux,
+ macOS, CI, and the Scala engine rely on is left untouched (see #4409).
+ """
+ if not warehouse:
+ return False
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return True
+ if text.startswith("file://"):
+ return bool(_WINDOWS_DRIVE_PATH.match(urlparse(text).path.lstrip("/")))
+ return False
Review Comment:
Good catch — switched to `urlparse(...).scheme.lower() == "file"` in 900f2eb so an uppercase `FILE:///C:/...` is recognized too, with a test to lock it in.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6545:
URL: https://github.com/apache/texera/pull/6545#issuecomment-5014193286
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 7 worse · ⚪ 8 noise (<±5%) · 0 without baseline
> Compared against main `193b7d4` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/29672758790)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 466 | 0.284 | 21,860/30,198/30,198 us | 🔴 +26.0% / 🔴 +93.9% |
| ⚪ | bs=100 sw=10 sl=64 | 1,183 | 0.722 | 83,201/106,457/106,457 us | ⚪ within ±5% / 🟢 +18.2% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,372 | 0.838 | 725,490/830,360/830,360 us | 🔴 +10.3% / 🟢 +33.4% |
<details><summary>Baseline details</summary>
Latest main `193b7d4` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 466 tuples/sec | 559 tuples/sec | 768.42 tuples/sec | -16.6% | -39.4% |
| bs=10 sw=10 sl=64 | MB/s | 0.284 MB/s | 0.341 MB/s | 0.469 MB/s | -16.7% | -39.4% |
| bs=10 sw=10 sl=64 | p50 | 21,860 us | 17,348 us | 12,686 us | +26.0% | +72.3% |
| bs=10 sw=10 sl=64 | p95 | 30,198 us | 25,538 us | 15,574 us | +18.2% | +93.9% |
| bs=10 sw=10 sl=64 | p99 | 30,198 us | 25,538 us | 20,076 us | +18.2% | +50.4% |
| bs=100 sw=10 sl=64 | throughput | 1,183 tuples/sec | 1,177 tuples/sec | 1,001 tuples/sec | +0.5% | +18.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.722 MB/s | 0.718 MB/s | 0.611 MB/s | +0.6% | +18.2% |
| bs=100 sw=10 sl=64 | p50 | 83,201 us | 82,573 us | 99,753 us | +0.8% | -16.6% |
| bs=100 sw=10 sl=64 | p95 | 106,457 us | 109,576 us | 106,344 us | -2.8% | +0.1% |
| bs=100 sw=10 sl=64 | p99 | 106,457 us | 109,576 us | 119,585 us | -2.8% | -11.0% |
| bs=1000 sw=10 sl=64 | throughput | 1,372 tuples/sec | 1,411 tuples/sec | 1,029 tuples/sec | -2.8% | +33.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.838 MB/s | 0.861 MB/s | 0.628 MB/s | -2.7% | +33.4% |
| bs=1000 sw=10 sl=64 | p50 | 725,490 us | 710,872 us | 979,178 us | +2.1% | -25.9% |
| bs=1000 sw=10 sl=64 | p95 | 830,360 us | 752,907 us | 1,021,692 us | +10.3% | -18.7% |
| bs=1000 sw=10 sl=64 | p99 | 830,360 us | 752,907 us | 1,055,014 us | +10.3% | -21.3% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,429.37,200,128000,466,0.284,21860.27,30197.99,30197.99
1,100,10,64,20,1690.51,2000,1280000,1183,0.722,83201.37,106456.66,106456.66
2,1000,10,64,20,14573.82,20000,12800000,1372,0.838,725490.26,830359.55,830359.55
```
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6545:
URL: https://github.com/apache/texera/pull/6545#issuecomment-5014171511
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@mengw15`
You can notify them by mentioning `@mengw15` in a comment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #6545:
URL: https://github.com/apache/texera/pull/6545
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #6545:
URL: https://github.com/apache/texera/pull/6545#discussion_r3609834330
##########
amber/src/main/python/core/storage/iceberg/iceberg_utils.py:
##########
@@ -35,6 +39,58 @@
# Suffix used to encode LARGE_BINARY fields in Iceberg (must match Scala IcebergUtil)
LARGE_BINARY_FIELD_SUFFIX = "__texera_large_binary_ptr"
+# pyiceberg FileIO whose fsspec LocalFileSystem handles Windows drive paths,
+# which the default pyarrow FileIO cannot. See `_is_windows_local_warehouse`.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
+# Matches a filesystem path that starts with a Windows drive letter and a
+# separator, e.g. `C:\...` or `C:/...` (but not the drive-relative `C:foo`).
+_WINDOWS_DRIVE_PATH = re.compile(r"^[A-Za-z]:[\\/]")
+
+
+def _is_windows_local_warehouse(warehouse: str) -> bool:
+ """
+ Whether ``warehouse`` is a LOCAL filesystem path carrying a Windows drive
+ letter, either as a bare path (``C:\\...`` / ``C:/...``) or a ``file://``
+ URI (``file:///C:/...``).
+
+ Remote object stores (``s3://...``), POSIX paths, drive-less relative paths,
+ and colon-stripped paths (the form the Scala side registers, ``C/...``) are
+ excluded, so the default pyarrow FileIO / plain-path convention that Linux,
+ macOS, CI, and the Scala engine rely on is left untouched (see #4409).
+ """
+ if not warehouse:
+ return False
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return True
+ parsed = urlparse(text)
+ if parsed.scheme.lower() == "file": # `file` URI schemes are case-insensitive
+ return bool(_WINDOWS_DRIVE_PATH.match(parsed.path.lstrip("/")))
+ return False
+
+
+def _to_file_uri(warehouse: str) -> str:
+ """
+ Normalize a bare Windows drive path (``C:\\...``) to a ``file:///`` URI so
+ pyiceberg does not mis-parse the drive letter as a URI scheme (``c://`` ->
+ "Unrecognized filesystem type in URI: c"). Values that are already URIs
+ (e.g. ``file:///C:/...``) are returned unchanged.
+
+ The URI is built from ``PureWindowsPath.as_posix()`` rather than
+ ``.as_uri()`` so the path is *not* percent-encoded: the fsspec
+ ``LocalFileSystem`` behind ``FsspecFileIO`` does not URL-decode, so an
+ encoded space (``C:\\Users\\John Doe`` -> ``.../John%20Doe``) would send
+ writes to a literally ``%20``-named directory. ``PureWindowsPath`` keeps the
+ conversion deterministic on every host OS (the result is identical off
+ Windows).
+ """
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return "file:///" + PureWindowsPath(text).as_posix()
Review Comment:
The code (and the space test) use `as_posix()`, but the fix table in the PR description still says the URI is built "via `PureWindowsPath.as_uri()`" — a leftover from an earlier iteration that now contradicts the paragraph right below it. Worth refreshing before merge since the description lands in the squash commit message.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): run Iceberg local storage on Windows in the Python worker [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6545:
URL: https://github.com/apache/texera/pull/6545#discussion_r3609792951
##########
amber/src/test/python/core/storage/iceberg/test_iceberg_utils_catalog.py:
##########
@@ -96,3 +100,141 @@ def test_postgres_uri_is_built_with_pg8000_scheme(self):
)
# And warehouse is still the plain path.
assert kwargs["warehouse"] == warehouse_path
+
+
+# FileIO selected for Windows-local warehouses in place of the default pyarrow
+# FileIO; its fsspec LocalFileSystem parses Windows drive paths.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
Review Comment:
The test hard-codes the FsspecFileIO implementation string, duplicating the production constant in `iceberg_utils.py`. This can drift if the implementation path changes; tests can instead reference the module constant.
##########
amber/src/main/python/core/storage/iceberg/iceberg_utils.py:
##########
@@ -35,6 +39,57 @@
# Suffix used to encode LARGE_BINARY fields in Iceberg (must match Scala IcebergUtil)
LARGE_BINARY_FIELD_SUFFIX = "__texera_large_binary_ptr"
+# pyiceberg FileIO whose fsspec LocalFileSystem handles Windows drive paths,
+# which the default pyarrow FileIO cannot. See `_is_windows_local_warehouse`.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
+# Matches a filesystem path that starts with a Windows drive letter and a
+# separator, e.g. `C:\...` or `C:/...` (but not the drive-relative `C:foo`).
+_WINDOWS_DRIVE_PATH = re.compile(r"^[A-Za-z]:[\\/]")
+
+
+def _is_windows_local_warehouse(warehouse: str) -> bool:
+ """
+ Whether ``warehouse`` is a LOCAL filesystem path carrying a Windows drive
+ letter, either as a bare path (``C:\\...`` / ``C:/...``) or a ``file://``
+ URI (``file:///C:/...``).
+
+ Remote object stores (``s3://...``), POSIX paths, drive-less relative paths,
+ and colon-stripped paths (the form the Scala side registers, ``C/...``) are
+ excluded, so the default pyarrow FileIO / plain-path convention that Linux,
+ macOS, CI, and the Scala engine rely on is left untouched (see #4409).
+ """
+ if not warehouse:
+ return False
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return True
+ if text.startswith("file://"):
+ return bool(_WINDOWS_DRIVE_PATH.match(urlparse(text).path.lstrip("/")))
+ return False
Review Comment:
`file` URI schemes are case-insensitive, but `_is_windows_local_warehouse` only recognizes lowercase `file://` via `startswith`. Using `urlparse(...).scheme.lower() == "file"` makes the gate robust to inputs like `FILE:///C:/...` without widening it beyond file URIs.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org