Skip to content

Commit f93a5f4

Browse files
authored
[DataLoader] Add ArrivalOrder API and batch_size support (#537)
## Summary Re-introduce the `ArrivalOrder` scan order and `batch_size` parameter that were removed in #504. The original removal was necessary because the fork dependency (`sumedhsakdeo/iceberg-python`) made it ineligible for use internally at LI. Now that `li-pyiceberg==0.11.3` includes the `ArrivalOrder` API from upstream (apache/iceberg-python#3046), we can restore the functionality using an approved registry dependency. In a future PR, we will add support for streaming multiple splits concurrently using `ArrivalOrder` with `concurrent_streams > 1`. ## Changes - [x] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests **`pyproject.toml`** — Bump `li-pyiceberg` from `0.11.2` to `0.11.3` which includes the `ArrivalOrder` API. **`data_loader.py`** — Re-add `batch_size` parameter to `OpenHouseDataLoader.__init__`, forwarded to each `DataLoaderSplit`. **`data_loader_split.py`** — Re-add `ArrivalOrder` import and `batch_size` parameter. `to_record_batches()` now uses `order=ArrivalOrder(concurrent_streams=1, batch_size=self._batch_size)`. **`uv.lock`** — Regenerated to resolve `li-pyiceberg==0.11.3` from the internal registry. **Tests** — Re-add `test_arrival_order.py` (verifies `ScanOrder` class hierarchy and `ArrowScan.to_record_batches` with `order` parameter). Re-add `batch_size` tests in `test_data_loader.py`, `test_data_loader_split.py`, and `integration_tests.py`. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] Added new tests for the changes made. - [x] Updated existing tests to reflect the changes made. - [ ] No tests added or updated. Please explain why. If unsure, please feel free to ask for help. - [ ] Some other form of testing like staging or soak time in production. Please explain. `make verify` passes — 202 tests pass, lint, format, and mypy all green. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [ ] Large PR broken into smaller PRs, and PR plan linked in the description. `batch_size` is re-added to `OpenHouseDataLoader` and `DataLoaderSplit`. This is additive (new optional parameter with default `None`), so existing callers are unaffected.
1 parent 123371d commit f93a5f4

8 files changed

Lines changed: 252 additions & 24 deletions

File tree

integrations/python/dataloader/pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ readme = "README.md"
1010
requires-python = ">=3.12"
1111
license = {text = "BSD-2-Clause"}
1212
keywords = ["openhouse", "data-loader", "lakehouse", "iceberg", "datafusion"]
13-
dependencies = ["datafusion==51.0.0", "li-pyiceberg==0.11.2", "requests>=2.31.0", "sqlglot>=29.0.0", "tenacity>=8.0.0"]
13+
dependencies = ["datafusion==51.0.0", "li-pyiceberg==0.11.3", "requests>=2.31.0", "sqlglot>=29.0.0", "tenacity>=8.0.0"]
1414

1515
[[tool.uv.index]]
1616
url = "https://linkedin.jfrog.io/artifactory/api/pypi/openhouse-pypi/simple/"

integrations/python/dataloader/src/openhouse/dataloader/data_loader.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ def __init__(
114114
filters: Filter | None = None,
115115
context: DataLoaderContext | None = None,
116116
max_attempts: int = 3,
117+
batch_size: int | None = None,
117118
):
118119
"""
119120
Args:
@@ -126,6 +127,10 @@ def __init__(
126127
filters: Row filter expression, defaults to always_true() (all rows)
127128
context: Data loader context
128129
max_attempts: Total number of attempts including the initial try (default 3)
130+
batch_size: Maximum number of rows per RecordBatch yielded by each split.
131+
Passed to PyArrow's Scanner which produces batches of at most this many
132+
rows. Smaller values reduce peak memory but increase per-batch overhead.
133+
None uses the PyArrow default (~131K rows).
129134
"""
130135
if branch is not None and branch.strip() == "":
131136
raise ValueError("branch must not be empty or whitespace")
@@ -138,6 +143,7 @@ def __init__(
138143
self._filters = filters if filters is not None else always_true()
139144
self._context = context or DataLoaderContext()
140145
self._max_attempts = max_attempts
146+
self._batch_size = batch_size
141147

142148
if self._context.jvm_config is not None and self._context.jvm_config.planner_args is not None:
143149
apply_libhdfs_opts(self._context.jvm_config.planner_args)
@@ -260,4 +266,5 @@ def __iter__(self) -> Iterator[DataLoaderSplit]:
260266
scan_context=scan_context,
261267
transform_sql=optimized_sql,
262268
udf_registry=self._context.udf_registry,
269+
batch_size=self._batch_size,
263270
)

integrations/python/dataloader/src/openhouse/dataloader/data_loader_split.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
from datafusion.context import SessionContext
88
from pyarrow import RecordBatch
99
from pyiceberg.io.pyarrow import ArrowScan
10-
from pyiceberg.table import FileScanTask
10+
from pyiceberg.table import ArrivalOrder, FileScanTask
1111

1212
from openhouse.dataloader._jvm import apply_libhdfs_opts
1313
from openhouse.dataloader._table_scan_context import TableScanContext
@@ -53,11 +53,13 @@ def __init__(
5353
scan_context: TableScanContext,
5454
transform_sql: str | None = None,
5555
udf_registry: UDFRegistry | None = None,
56+
batch_size: int | None = None,
5657
):
5758
self._file_scan_task = file_scan_task
5859
self._scan_context = scan_context
5960
self._transform_sql = transform_sql
6061
self._udf_registry = udf_registry or NoOpRegistry()
62+
self._batch_size = batch_size
6163

6264
@property
6365
def id(self) -> str:
@@ -76,7 +78,8 @@ def __iter__(self) -> Iterator[RecordBatch]:
7678
"""Reads the file scan task and yields Arrow RecordBatches.
7779
7880
Uses PyIceberg's ArrowScan to handle format dispatch, schema resolution,
79-
delete files, and partition spec lookups.
81+
delete files, and partition spec lookups. The number of batches loaded
82+
into memory at once is bounded to prevent using too much memory at once.
8083
"""
8184
ctx = self._scan_context
8285
if ctx.worker_jvm_args is not None:
@@ -88,7 +91,10 @@ def __iter__(self) -> Iterator[RecordBatch]:
8891
row_filter=ctx.row_filter,
8992
)
9093

91-
batches = arrow_scan.to_record_batches([self._file_scan_task])
94+
batches = arrow_scan.to_record_batches(
95+
[self._file_scan_task],
96+
order=ArrivalOrder(concurrent_streams=1, batch_size=self._batch_size),
97+
)
9298

9399
if self._transform_sql is None:
94100
yield from batches

integrations/python/dataloader/tests/integration_tests.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,13 @@ def read_token() -> str:
228228
snap1 = OpenHouseDataLoader(catalog=catalog, database=DATABASE_ID, table=TABLE_ID).snapshot_id
229229
assert snap1 is not None
230230

231-
# 4. Read all data
232-
result = _read_all(OpenHouseDataLoader(catalog=catalog, database=DATABASE_ID, table=TABLE_ID))
231+
# 4. Read all data with batch_size and verify batch count
232+
loader = OpenHouseDataLoader(catalog=catalog, database=DATABASE_ID, table=TABLE_ID, batch_size=2)
233+
batches = [batch for split in loader for batch in split]
234+
assert len(batches) == 2, f"Expected 2 batches (3 rows, batch_size=2), got {len(batches)}"
235+
for batch in batches:
236+
assert batch.num_rows <= 2
237+
result = pa.concat_tables([pa.Table.from_batches([b]) for b in batches]).sort_by(COL_ID)
233238
finally:
234239
os.dup2(saved_stdout, 1)
235240
os.close(saved_stdout)
@@ -240,7 +245,7 @@ def read_token() -> str:
240245
assert result.column(COL_ID).to_pylist() == [1, 2, 3]
241246
assert result.column(COL_NAME).to_pylist() == ["alice", "bob", "charlie"]
242247
assert result.column(COL_SCORE).to_pylist() == [1.1, 2.2, 3.3]
243-
print(f"PASS: read all {result.num_rows} rows")
248+
print(f"PASS: read all {result.num_rows} rows in {len(batches)} batches (batch_size=2)")
244249

245250
# 5a. Row filter
246251
loader = OpenHouseDataLoader(catalog=catalog, database=DATABASE_ID, table=TABLE_ID, filters=col(COL_ID) > 1)
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
"""Tests verifying the ArrivalOrder API from pyiceberg PR #3046 is available and functional.
2+
3+
These tests confirm that the openhouse dataloader can access the new ScanOrder class hierarchy
4+
added upstream (apache/iceberg-python#3046) and that ArrowScan.to_record_batches accepts the
5+
order parameter.
6+
"""
7+
8+
import os
9+
10+
import pyarrow as pa
11+
import pyarrow.parquet as pq
12+
import pytest
13+
from pyiceberg.expressions import AlwaysTrue
14+
from pyiceberg.io import load_file_io
15+
from pyiceberg.io.pyarrow import ArrowScan
16+
from pyiceberg.manifest import DataFile, FileFormat
17+
from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC
18+
from pyiceberg.schema import Schema
19+
from pyiceberg.table import ArrivalOrder, FileScanTask, ScanOrder, TaskOrder
20+
from pyiceberg.table.metadata import new_table_metadata
21+
from pyiceberg.table.sorting import UNSORTED_SORT_ORDER
22+
from pyiceberg.types import LongType, NestedField, StringType
23+
24+
_SCHEMA = Schema(
25+
NestedField(field_id=1, name="id", field_type=LongType(), required=False),
26+
NestedField(field_id=2, name="name", field_type=StringType(), required=False),
27+
)
28+
29+
30+
def _write_parquet(tmp_path: object, table: pa.Table) -> str:
31+
"""Write a parquet file with Iceberg field IDs and return its path."""
32+
file_path = str(tmp_path / "test.parquet") # type: ignore[operator]
33+
fields = [field.with_metadata({b"PARQUET:field_id": str(i + 1).encode()}) for i, field in enumerate(table.schema)]
34+
pq.write_table(table.cast(pa.schema(fields)), file_path)
35+
return file_path
36+
37+
38+
def _make_arrow_scan(tmp_path: object, file_path: str) -> ArrowScan:
39+
metadata = new_table_metadata(
40+
schema=_SCHEMA,
41+
partition_spec=UNPARTITIONED_PARTITION_SPEC,
42+
sort_order=UNSORTED_SORT_ORDER,
43+
location=str(tmp_path),
44+
properties={},
45+
)
46+
return ArrowScan(
47+
table_metadata=metadata,
48+
io=load_file_io(properties={}, location=file_path),
49+
projected_schema=_SCHEMA,
50+
row_filter=AlwaysTrue(),
51+
)
52+
53+
54+
def _make_file_scan_task(file_path: str, table: pa.Table) -> FileScanTask:
55+
data_file = DataFile.from_args(
56+
file_path=file_path,
57+
file_format=FileFormat.PARQUET,
58+
record_count=table.num_rows,
59+
file_size_in_bytes=os.path.getsize(file_path),
60+
)
61+
data_file._spec_id = 0
62+
return FileScanTask(data_file=data_file)
63+
64+
65+
def _sample_table() -> pa.Table:
66+
return pa.table(
67+
{
68+
"id": pa.array([1, 2, 3], type=pa.int64()),
69+
"name": pa.array(["alice", "bob", "charlie"], type=pa.string()),
70+
}
71+
)
72+
73+
74+
class TestScanOrderImports:
75+
"""Verify the ScanOrder class hierarchy is importable from pyiceberg.table."""
76+
77+
def test_scan_order_base_class_exists(self) -> None:
78+
assert ScanOrder is not None
79+
80+
def test_task_order_is_scan_order(self) -> None:
81+
assert issubclass(TaskOrder, ScanOrder)
82+
83+
def test_arrival_order_is_scan_order(self) -> None:
84+
assert issubclass(ArrivalOrder, ScanOrder)
85+
86+
def test_arrival_order_default_params(self) -> None:
87+
ao = ArrivalOrder()
88+
assert ao.concurrent_streams == 8
89+
assert ao.batch_size is None
90+
assert ao.max_buffered_batches == 16
91+
92+
def test_arrival_order_custom_params(self) -> None:
93+
ao = ArrivalOrder(concurrent_streams=4, batch_size=32768, max_buffered_batches=8)
94+
assert ao.concurrent_streams == 4
95+
assert ao.batch_size == 32768
96+
assert ao.max_buffered_batches == 8
97+
98+
def test_arrival_order_rejects_invalid_concurrent_streams(self) -> None:
99+
with pytest.raises(ValueError, match="concurrent_streams"):
100+
ArrivalOrder(concurrent_streams=0)
101+
102+
def test_arrival_order_rejects_invalid_max_buffered_batches(self) -> None:
103+
with pytest.raises(ValueError, match="max_buffered_batches"):
104+
ArrivalOrder(max_buffered_batches=0)
105+
106+
107+
class TestToRecordBatchesOrder:
108+
"""Verify ArrowScan.to_record_batches accepts the order parameter and returns correct data."""
109+
110+
def test_default_order_returns_all_rows(self, tmp_path: object) -> None:
111+
"""Default (TaskOrder) still works — backward compatible."""
112+
table = _sample_table()
113+
file_path = _write_parquet(tmp_path, table)
114+
arrow_scan = _make_arrow_scan(tmp_path, file_path)
115+
task = _make_file_scan_task(file_path, table)
116+
batches = list(arrow_scan.to_record_batches([task]))
117+
result = pa.Table.from_batches(batches).sort_by("id")
118+
assert result.column("id").to_pylist() == [1, 2, 3]
119+
120+
def test_explicit_task_order_returns_all_rows(self, tmp_path: object) -> None:
121+
table = _sample_table()
122+
file_path = _write_parquet(tmp_path, table)
123+
arrow_scan = _make_arrow_scan(tmp_path, file_path)
124+
task = _make_file_scan_task(file_path, table)
125+
batches = list(arrow_scan.to_record_batches([task], order=TaskOrder()))
126+
result = pa.Table.from_batches(batches).sort_by("id")
127+
assert result.column("id").to_pylist() == [1, 2, 3]
128+
129+
def test_arrival_order_returns_all_rows(self, tmp_path: object) -> None:
130+
table = _sample_table()
131+
file_path = _write_parquet(tmp_path, table)
132+
arrow_scan = _make_arrow_scan(tmp_path, file_path)
133+
task = _make_file_scan_task(file_path, table)
134+
batches = list(arrow_scan.to_record_batches([task], order=ArrivalOrder(concurrent_streams=2)))
135+
result = pa.Table.from_batches(batches).sort_by("id")
136+
assert result.column("id").to_pylist() == [1, 2, 3]
137+
assert result.column("name").to_pylist() == ["alice", "bob", "charlie"]

integrations/python/dataloader/tests/test_data_loader.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,33 @@ def fake_scan(**kwargs):
567567
assert branch_splits[0]._file_scan_task.file.file_path == "branch.parquet"
568568

569569

570+
# --- batch_size tests ---
571+
572+
573+
def test_batch_size_forwarded_to_splits(tmp_path):
574+
"""batch_size is correctly passed through to each DataLoaderSplit."""
575+
catalog = _make_real_catalog(tmp_path)
576+
577+
loader = OpenHouseDataLoader(catalog=catalog, database="db", table="tbl", batch_size=32768)
578+
splits = list(loader)
579+
580+
assert len(splits) >= 1
581+
for split in splits:
582+
assert split._batch_size == 32768
583+
584+
585+
def test_batch_size_default_is_none(tmp_path):
586+
"""Omitting batch_size defaults to None in each split."""
587+
catalog = _make_real_catalog(tmp_path)
588+
589+
loader = OpenHouseDataLoader(catalog=catalog, database="db", table="tbl")
590+
splits = list(loader)
591+
592+
assert len(splits) >= 1
593+
for split in splits:
594+
assert split._batch_size is None
595+
596+
570597
# --- Predicate pushdown with transformer tests ---
571598

572599

integrations/python/dataloader/tests/test_data_loader_split.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ def _create_test_split(
4343
transform_sql: str | None = None,
4444
table_id: TableIdentifier = _DEFAULT_TABLE_ID,
4545
udf_registry: UDFRegistry | None = None,
46+
batch_size: int | None = None,
4647
) -> DataLoaderSplit:
4748
"""Create a DataLoaderSplit for testing by writing data to disk.
4849
@@ -103,6 +104,7 @@ def _create_test_split(
103104
scan_context=scan_context,
104105
transform_sql=transform_sql,
105106
udf_registry=udf_registry,
107+
batch_size=batch_size,
106108
)
107109

108110

@@ -422,3 +424,47 @@ def test_worker_jvm_args_sets_libhdfs_opts(tmp_path, monkeypatch):
422424
list(split)
423425

424426
assert os.environ[LIBHDFS_OPTS_ENV] == "-Xmx512m"
427+
428+
429+
# --- batch_size tests ---
430+
431+
_BATCH_SCHEMA = Schema(
432+
NestedField(field_id=1, name="id", field_type=LongType(), required=False),
433+
)
434+
435+
436+
def _make_table(num_rows: int) -> pa.Table:
437+
return pa.table({"id": pa.array(list(range(num_rows)), type=pa.int64())})
438+
439+
440+
def test_split_batch_size_limits_rows_per_batch(tmp_path):
441+
"""When batch_size is set, each RecordBatch has at most that many rows."""
442+
table = _make_table(100)
443+
split = _create_test_split(tmp_path, table, FileFormat.PARQUET, _BATCH_SCHEMA, batch_size=10)
444+
445+
batches = list(split)
446+
447+
assert len(batches) >= 2, "Expected multiple batches with batch_size=10 and 100 rows"
448+
for batch in batches:
449+
assert batch.num_rows <= 10
450+
assert sum(b.num_rows for b in batches) == 100
451+
452+
453+
def test_split_batch_size_none_returns_all_rows(tmp_path):
454+
"""Default batch_size (None) returns all data correctly."""
455+
table = _make_table(50)
456+
split = _create_test_split(tmp_path, table, FileFormat.PARQUET, _BATCH_SCHEMA)
457+
458+
result = pa.Table.from_batches(list(split))
459+
assert result.num_rows == 50
460+
assert sorted(result.column("id").to_pylist()) == list(range(50))
461+
462+
463+
def test_split_batch_size_preserves_data(tmp_path):
464+
"""batch_size controls chunking but all data is preserved."""
465+
table = _make_table(25)
466+
split = _create_test_split(tmp_path, table, FileFormat.PARQUET, _BATCH_SCHEMA, batch_size=7)
467+
468+
result = pa.Table.from_batches(list(split))
469+
assert result.num_rows == 25
470+
assert sorted(result.column("id").to_pylist()) == list(range(25))

0 commit comments

Comments
 (0)