Only override the arrow1 versions of the Loggable trait #8604
Conversation
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
| // TODO(cmc): We use the extended type here because we rely on it for formatting. | ||
| Ok(StructArray::new(datatype, values, validity).boxed()) |
There was a problem hiding this comment.
@teh-cmc this will surely cause a regression, but I don't know where.
There was a problem hiding this comment.
(I haven't reviewed the rest of the PR so I might be lacking context)
This is the one exception where we really really want to keep the extension around, otherwise printing any chunk becomes awful, which makes day-to-day life very hard.
Any reason you cannot keep it around?
There was a problem hiding this comment.
There is no extended types in arrow-rd:
I can set ARROW:extension:name: rerun.datatypes.TUID in the metadata on the fields (but which field?), but I need some way to test that I'm not breaking anything. I don't know where we rely on this.
AFAICT we use it when displaying a ChunkId or RowId in the UI… which I'm not sure we do?
There was a problem hiding this comment.
We do show ChunkId:s the chunk store browser, and everything is fine there:
There was a problem hiding this comment.
Manually adding an ARROW:extension:name in the TransportChunk glue seems like the right solution yes.
The best way to test for this is Command Palette > Print datastore. Right now your PR doesn't break it (probably because that still goes through arrow2?), so we can see about that later.
| @@ -23,115 +23,113 @@ use crate::{Archetype, ComponentBatch, LoggableBatch}; | |||
| /// which makes it possible to work with lists' worth of data in a generic fashion. | |||
| pub trait Loggable: 'static + Send + Sync + Clone + Sized + SizeBytes { | |||
There was a problem hiding this comment.
I moved around the functions to group them better (arrow2 stuff at the end). Sorry.
emilk
force-pushed
the
emilk/remove-arrow2-loggable
branch
from
740bd2e to
a324a3b
Compare
emilk
added
🏹 arrow
emilk
force-pushed
the
emilk/remove-arrow2-loggable
branch
from
23cbb36 to
026b310
Compare
|
@rerun-bot full-check |
|
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12658242886 |
Related
re_arrow2toarrow#3741What
All the arrow2-functions on
trait Loggablenow defer to the arrow1 implementations, and all implementations only override the arrow1-trait functions.