feat(datasets): add Apache Iceberg dataset support for polars - #1503
feat(datasets): add Apache Iceberg dataset support for polars#1503Saurav-Gupta-9741 wants to merge 1 commit into
Conversation
a4a0341 to
7297b1d
Compare
eaff45d to
c0a3d8a
Compare
|
@Saurav-Gupta-9741 please reduce this to just one library--which library are you primarily working with? @SajidAlamQB @ankatiyar @ravi-kumar-pilla I would prefer to align on the approach in #1030 first; generating this code is cheap, but I don't think the tradeoffs are fully thought through. (@Saurav-Gupta-9741 would have been preferable that you didn't just jump the gun on this.) |
c0a3d8a to
b1d036a
Compare
|
Hi @deepyaman Sir |
|
Hi @Saurav-Gupta-9741 , Thanks for putting this together, and for narrowing it down to Polars after Deepyaman’s feedback. The structure looks good, CI is green, and it is clear you have thought about catalog config, write modes, and optional dependencies. Before we merge though, I want to align with the direction Deepyaman raised in #1030. I think the goal should be a thin Kedro wrapper around Polars native Iceberg APIs, not a PyIceberg shim inside Right now Load: delegate to Save: delegate to That keeps filter pushdown, streaming, and future Polars Iceberg improvements on the native path instead of maintaining a parallel PyIceberg layer in Kedro. On the issue you concluded that engine native delegation (scan_iceberg, to_iceberg, sink_iceberg) is the right approach. The current PR goes back to a PyIceberg direct implementation instead. Can you explain what changed? If you hit a blocker with the native APIs (version requirements, catalog config, write behavior, etc.) please share that here. If not, I think we should refactor to match what you proposed on #1030. A few other things to tidy up regardless of the above:
Let me know if refactoring to native Polars APIs raises any concerns on your side. Thanks again for the contribution. |
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
Left some comments here - #1503 (comment)
ec8ce13 to
5119f33
Compare
|
Hi @ravi-kumar-pilla Sir, Thank you so much for the detailed and thoughtful review! I have addressed all your points and updated the PR accordingly: 1. Polars Native Delegation (
|
|
Could you also update the PR description to just focus on the one dataset? |
2f4b66a to
06eb459
Compare
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
Thanks for the updates. Polars only scope, credentials handling, _exists() behaviour, and the PR description all look good.
Two things to fix before merge:
- Module and class docstrings overstate Polars native coverage. Save goes through PyIceberg, not Polars write APIs.
load_argsmixes Polars and PyIceberg scan options in one dict. Invalid keys on the native path get silently swallowed.
| @@ -0,0 +1,204 @@ | |||
| """``IcebergDataset`` loads and saves data from/to Apache Iceberg tables | |||
There was a problem hiding this comment.
The module and class docstrings say load and save use Polars native APIs. That is only partly true.
Load tries scan_iceberg but falls back to PyIceberg. Save does not use write_iceberg or sink_iceberg today. It goes through PyIceberg directly.
Please update both docstrings. Something like:
"""``IcebergDataset`` loads and saves Apache Iceberg tables.
Loads via Polars ``scan_iceberg`` when available, with a PyIceberg scan fallback.
Saves commit data to Iceberg tables. Catalog access is handled through PyIceberg.
"""Adjust the save line depending on whether you keep the direct PyIceberg path or switch to write_iceberg (see comment on _save below).
| credentials: Authentication credentials or secrets (e.g. tokens, AWS/GCP keys). | ||
| These are merged with ``catalog_properties`` when connecting to the catalog | ||
| and are safely excluded from ``_describe()`` to avoid leaking secrets. | ||
| load_args: Additional scan/read options passed to ``polars.scan_iceberg`` |
There was a problem hiding this comment.
load_args documents options for two different APIs in one dict.
polars.scan_iceberg accepts snapshot_id, storage_options, reader_override, etc. PyIceberg table.scan accepts selected_fields, row_filter, limit. These are not interchangeable.
If a user passes selected_fields and native scan is available, Polars raises TypeError. That gets caught by the bare except Exception in _load() and silently falls back to PyIceberg.
I would restrict load_args to Polars native keys only. Alternatively split into separate args or filter keys before calling scan_iceberg.
| try: | ||
| lazy_df = pl.scan_iceberg(table, **self._load_args) | ||
| return lazy_df.collect() | ||
| except Exception: # noqa: BLE001 |
There was a problem hiding this comment.
The bare except Exception: pass in _load() can hide real errors. Bad snapshot_id, auth failures, and invalid load_args all get swallowed.
Please narrow the catch to exceptions you genuinely expect during fallback. Re raise on TypeError and ValueError from bad user args.
| arrow_table = scan.to_arrow() | ||
| return pl.from_arrow(arrow_table) | ||
|
|
||
| def _save(self, data: pl.DataFrame | pl.LazyFrame) -> None: |
There was a problem hiding this comment.
For save, consider data.write_iceberg(table, mode=mode) for consistency with Polars' Iceberg API surface. Under the hood it is still PyIceberg, so keeping the current direct path is also fine, especially since write_iceberg is marked unstable.
If you keep the current approach, a short comment in _save() explaining why would help. Update the docstring save line to match whichever approach you go with.
| Args: | ||
| table_name: Table identifier (e.g. ``"namespace.table_name"`` or ``"table_name"``). | ||
| catalog_name: Name of the Iceberg catalog to load. Defaults to None. | ||
| catalog_properties: Properties required to instantiate the catalog (e.g. |
There was a problem hiding this comment.
credentials are correctly excluded from _describe(). Secrets in catalog_properties will still show up in _describe(). Worth a note on the catalog_properties arg: do not put secrets here, use credentials instead.
| @@ -0,0 +1,208 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Mock tests are a good start. No explicit test for the native scan_iceberg path. test_load_pyiceberg_scan may hit the fallback depending on Polars version.
PR description says 12 tests, file has 13.
| --ignore kedro_datasets/huggingface/transformer_pipeline_dataset.py \ | ||
| --ignore kedro_datasets/pandas/gbq_dataset.py \ | ||
| --ignore kedro_datasets/partitions/partitioned_dataset.py \ | ||
| --ignore kedro_datasets/polars/iceberg_dataset.py \ |
There was a problem hiding this comment.
Please add a short comment on the iceberg_dataset.py ignore entry explaining why (optional deps not available in doctest env).
06eb459 to
723a4a1
Compare
Add polars.IcebergDataset for reading/writing Apache Iceberg tables via Polars native scan_iceberg with PyIceberg catalog fallback and secure credentials handling. Closes kedro-org#1030 Signed-off-by: Saurav Gupta <91198524+Saurav-Gupta-13@users.noreply.github.com>
723a4a1 to
e01074f
Compare
Description & Motivation
Closes #1030.
This PR introduces native Apache Iceberg dataset support for Polars in
kedro-datasets:polars.IcebergDataset(kedro_datasets.polars.IcebergDataset)Key Capabilities:
polars.scan_iceberg(table, **load_args).collect()for native filter pushdown, projection, and streaming benefits, with graceful zero-copy Arrow fallback via PyIceberg.pyiceberg.catalog.load_catalog().credentialsparameter for catalog and storage authentication (e.g. tokens, access keys) while safely excluding credentials from_describe().overwrite(default) andappendcommit modes for eagerpl.DataFrameandpl.LazyFrame.How Has This Been Tested?
_describe()_exists()(NoSuchTableError,NoSuchNamespaceError)ruff.Checklist
ruff check,ruff format)RELEASE.md