Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small scan-handler fixes #16721

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Small scan-handler fixes #16721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01d08d1
Reject read from https:// paths
wence- Aug 30, 2024
6a0aba1
Raise if only reading row-index in parquet scan
wence- Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions python/cudf_polars/cudf_polars/dsl/ir.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ def __post_init__(self) -> None:
raise NotImplementedError(
"Read from cloud storage"
) # pragma: no cover; no test yet
if any(p.startswith("https://") for p in self.paths):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about http:// or other protocols?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way this occurs is because the user wrote scan_foo("hf://some_path/") and it is expanded before we see it into https://huggingface..../some_path.

So I could tighten up to match the full path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a broader brush to catch other URL-like things too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably fine. If no https:// URLs are supported then let’s not make it specific to HuggingFace.

raise NotImplementedError("Read from https")
if self.typ == "csv":
if self.reader_options["skip_rows_after_header"] != 0:
raise NotImplementedError("Skipping rows after header in CSV reader")
Expand Down Expand Up @@ -249,6 +251,15 @@ def __post_init__(self) -> None:
raise NotImplementedError(
"ignore_errors is not supported in the JSON reader"
)
elif (
self.typ == "parquet"
and self.row_index is not None
and self.with_columns is not None
and len(self.with_columns) == 0
):
raise NotImplementedError(
"Reading only parquet metadata to produce row index."
)

def evaluate(self, *, cache: MutableMapping[int, DataFrame]) -> DataFrame:
"""Evaluate and return a dataframe."""
Expand Down Expand Up @@ -365,12 +376,7 @@ def evaluate(self, *, cache: MutableMapping[int, DataFrame]) -> DataFrame:
raise NotImplementedError(
f"Unhandled scan type: {self.typ}"
) # pragma: no cover; post init trips first
if (
row_index is not None
# TODO: remove condition when dropping support for polars 1.0
# https://github.com/pola-rs/polars/pull/17363
and row_index[0] in self.schema
):
if row_index is not None:
name, offset = row_index
dtype = self.schema[name]
step = plc.interop.from_arrow(
Expand Down
11 changes: 11 additions & 0 deletions python/cudf_polars/tests/test_scan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,3 +315,14 @@ def test_scan_parquet_nested_null_raises(tmp_path):
q = pl.scan_parquet(tmp_path / "file.pq")

assert_ir_translation_raises(q, NotImplementedError)


def test_scan_parquet_only_row_index_raises(df, tmp_path):
make_source(df, tmp_path / "file", "parquet")
q = pl.scan_parquet(tmp_path / "file", row_index_name="index").select("index")
assert_ir_translation_raises(q, NotImplementedError)


def test_scan_hf_url_raises():
q = pl.scan_csv("hf://datasets/scikit-learn/iris/Iris.csv")
assert_ir_translation_raises(q, NotImplementedError)
Loading