close
Skip to content

Add V3 read support#1554

Merged
Fokko merged 1 commit into
apache:mainfrom
Fokko:fd-v3
Jan 21, 2025
Merged

Add V3 read support#1554
Fokko merged 1 commit into
apache:mainfrom
Fokko:fd-v3

Conversation

@Fokko
Copy link
Copy Markdown
Contributor

@Fokko Fokko commented Jan 21, 2025

Resolves #1550

@Fokko Fokko assigned Fokko and unassigned Fokko Jan 21, 2025
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

woot!

Comment thread tests/table/test_partitioning.py
@Fokko Fokko merged commit 5a3c346 into apache:main Jan 21, 2025
@Fokko Fokko deleted the fd-v3 branch January 21, 2025 20:39
@Fokko Fokko mentioned this pull request Jan 28, 2025
kevinjqliu pushed a commit that referenced this pull request Jan 30, 2025
@Fokko Fokko mentioned this pull request Mar 20, 2025
14 tasks
Fokko pushed a commit that referenced this pull request May 25, 2026
Inspired by the walrus issue in #3353

## Problem

`PartitionField` and `SortField` accept the v3 `source-ids` list (added
in [#1554](#1554)) and map
it to the legacy singular `source-id`. Both validators try to reject an
empty list:

```python
if "source-id" not in data and (source_ids := data["source-ids"]):
    if isinstance(source_ids, list):
        if len(source_ids) == 0:
            raise ValueError("Empty source-ids is not allowed")
        ...
        data["source-id"] = source_ids[0]
```

The walrus uses truthiness, and `[]` is falsy — so the `len(source_ids)
== 0` branch is unreachable. Passing `{"source-ids": []}` silently skips
the mapping, and Pydantic then reports a generic "field required" error
instead of the intended message. A missing `source-ids` key also raises
`KeyError` instead of being handled cleanly.

## Fix

Replace the walrus with an explicit key check in both validators:

```python
if "source-id" not in data and "source-ids" in data:
    source_ids = data["source-ids"]
    ...
```

This makes the empty-list validation reachable and avoids the
`KeyError`.

## Tests

Added regression tests that deserialize `{"source-ids": []}` and assert
`ValueError("Empty source-ids is not allowed")` is raised, for both
`PartitionField` and `SortField`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support reading V3 metadata

2 participants