-
Notifications
You must be signed in to change notification settings - Fork 22
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
Dying in specialization & generics #801
Comments
#803 removed i64 support. So we're making progress towards simplification |
kylebarron
added a commit
that referenced
this issue
Nov 14, 2024
In discussion with Dewey around this PR #844, I realized that regardless of whether we remove the underlying `CoordBuffer` enum, we can remove the const generic on the CoordBuffer to determine its physical dimension. Even if this is slightly slower, I think it's very important for maintainability. I realize now that this is what b4l was trying to argue for in #822, but I couldn't see what he meant there without an example. For #822, for #801
kylebarron
added a commit
that referenced
this issue
Nov 16, 2024
I think we can close this now! There are no generic parameters anymore! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm currently dying in generics.
It's making it harder to move fast, and gives a ton of development overhead. If there's one lesson I should learn from DuckDB Spatial, it's that I'm not moving fast enough.
Any algorithms that return float values, such as
area
, should only take in a&dyn NativeArray + NativeArrayAccessor<2>
.So we want something like an
NativeArrayAccessor<const D: usize>
, so that we can implement algorithms onThis also means we can greatly simplify our
IndexedArray
implementation too. We should switch toIndexedNativeArray
(or maybeIndexedNativeArray<const D: usize>
) which holds only an `Arc<>Also, anything like
area
that is implemented ongeo::Geometry
(orgeos::Geometry
) should be implemented solely on&dyn NativeArray + NativeArrayAccessor<2>
(or maybe allow 3d as well there) and not on every array type.The other issue here is that the
Geometry
scalar is generic over both dimension and the offset size of the underlying NativeArray. This is especially painful as the dimension matters for the Geometry but the offset size does not. It's only an artifact of the underlying storage.I'm getting closer to removing support internally for i64 offset arrays. We can support ingesting that data but only represent i32 data internally. To max out
i32
offsets, we'd need to have2^31 + 1
(2,147,483,649) coordinates, which would be 34,359,738,384 bytes, or 32GB.Especially the binary operations are a total disaster right now. This file is 750 lines of code just to implement intersections, when we aren't even doing any of the core implementation ourselves! This should be 10 lines of code, to take in a
&dyn NativeArray + NativeArrayAccessor<2>
, iterate over itsgeoarrow::scalar::Geometry<2>
objects, and callintersects
on each one.The text was updated successfully, but these errors were encountered: