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

Remove check for inner nullability of component columns #9009

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Conversation

emilk
Copy link
Member

@emilk emilk commented Feb 12, 2025

Related

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.

Copy link

github-actions bot commented Feb 12, 2025

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

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

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

@emilk emilk added 🪳 bug Something isn't working exclude from changelog PRs with this won't show up in CHANGELOG.md dataplatform Rerun Data Platform integration 🏹 arrow concerning arrow 🔩 data model Sorbet labels Feb 12, 2025
@emilk
Copy link
Member Author

emilk commented Feb 12, 2025

cargo nextest run --all-features --all-targets passes

@teh-cmc teh-cmc merged commit c9d8dc6 into main Feb 12, 2025
47 of 48 checks passed
@teh-cmc teh-cmc deleted the emilk/fix-redap branch February 12, 2025 14:13
return Err(ChunkError::Malformed {
reason: format!(
"The outer array in a chunked component batch must be a sparse list, got {:?}",
"The inner array in a chunked component batch must be a list, got {:?}",
Copy link
Member

Choose a reason for hiding this comment

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

This is not the inner array, this is the outer list_array, i.e. the column

Copy link
Member

Choose a reason for hiding this comment

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

But I already merged, so

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 🪳 bug Something isn't working 🔩 data model Sorbet dataplatform Rerun Data Platform integration exclude from changelog PRs with this won't show up in CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants