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.

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

Simplify and test wrap_in_list_array #8998

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Simplify and test wrap_in_list_array #8998

merged 2 commits into from
Feb 12, 2025

Conversation

emilk
Copy link
Member

@emilk emilk commented Feb 11, 2025

@emilk emilk added 🏹 arrow concerning arrow 🚜 refactor Change the code, not the functionality labels Feb 11, 2025
Copy link

github-actions bot commented Feb 11, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
86db620 https://rerun.io/viewer/pr/8998 +nightly +main

Note: This comment is updated whenever you push a commit.

@emilk emilk added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Feb 11, 2025
crates/utils/re_arrow_util/src/lib.rs Outdated Show resolved Hide resolved
crates/utils/re_arrow_util/src/lib.rs Show resolved Hide resolved
crates/utils/re_arrow_util/src/lib.rs Outdated Show resolved Hide resolved
@emilk emilk merged commit 5da23b9 into main Feb 12, 2025
35 checks passed
@emilk emilk deleted the emilk/wrap_in_list_array branch February 12, 2025 07:25
teh-cmc pushed a commit that referenced this pull request Feb 12, 2025
### Related
* #8998
* #6819

### What
`pixi run rerun rerun://redap.rerun.io/catalog` failed with `Detected
malformed Chunk: The outer array in chunked component batch must be a
sparse list, got List(Field { name: "item", data_type: Utf8, nullable:
false, dict_id: 0, dict_is_ordered: false, metadata: {} })`.

The cause was a check in the `Chunk::sanity_check` requiring _inner
mutability_, i.e. allowing a single component to be null in a list. The
intention of the check was to check for _outer_ nullability, i.e. that
any cell in the column can be `null` (or a dense list).

In other words, we want to support `Vec<Option<Vec<T>>`, but NOT require
`Vec<Option<Vec<Option<T>>>`.

Why did this trigger now? Because we now allow sending component columns
as "mono components", i.e. as `Vec<T>` and that is then automatically
changed to `Vec<Vec<T>>`, but wether or not it has interior nullability
depends on if the source data had it.
jprochazk pushed a commit that referenced this pull request Feb 13, 2025
jprochazk pushed a commit that referenced this pull request Feb 13, 2025
### Related
* #8998
* #6819

### What
`pixi run rerun rerun://redap.rerun.io/catalog` failed with `Detected
malformed Chunk: The outer array in chunked component batch must be a
sparse list, got List(Field { name: "item", data_type: Utf8, nullable:
false, dict_id: 0, dict_is_ordered: false, metadata: {} })`.

The cause was a check in the `Chunk::sanity_check` requiring _inner
mutability_, i.e. allowing a single component to be null in a list. The
intention of the check was to check for _outer_ nullability, i.e. that
any cell in the column can be `null` (or a dense list).

In other words, we want to support `Vec<Option<Vec<T>>`, but NOT require
`Vec<Option<Vec<Option<T>>>`.

Why did this trigger now? Because we now allow sending component columns
as "mono components", i.e. as `Vec<T>` and that is then automatically
changed to `Vec<Vec<T>>`, but wether or not it has interior nullability
depends on if the source data had it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏹 arrow concerning arrow exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants