You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "Kontinuation (via GitHub)" <gi...@apache.org> on 2026/02/10 13:40:12 UTC
[PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Kontinuation opened a new pull request, #589:
URL: https://github.com/apache/sedona-db/pull/589
## Summary
- Add three new `BandDataType` variants: `Int8` (14), `UInt64` (12), `Int64` (13) to match GDAL's GDALDataType ordinals
- Renumber existing `BandDataType` discriminants from 0-6 to 1-7 to align with GDAL's non-complex type ordinals (excluding GDT_Unknown=0)
- Derive `Hash` and `Copy` on both `BandDataType` and `StorageType` enums for ergonomic use
- Update match arms in `BandMetadataRefImpl::data_type()` to decode the new ordinals
- Extend test coverage and test data generators (`generate_random_band_data`, `get_nodata_value_for_type`) to handle all 10 band data types
## Stacked on
This PR is stacked on #588 (`pr2-better-band-metadata-ref`). Please merge #588 first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2797689058
##########
rust/sedona-raster/src/array.rs:
##########
@@ -146,13 +146,16 @@ impl<'a> BandMetadataRef for BandMetadataRefImpl<'a> {
fn data_type(&self) -> BandDataType {
match self.datatype_array.value(self.band_index) {
- 0 => BandDataType::UInt8,
- 1 => BandDataType::UInt16,
- 2 => BandDataType::Int16,
- 3 => BandDataType::UInt32,
- 4 => BandDataType::Int32,
- 5 => BandDataType::Float32,
- 6 => BandDataType::Float64,
+ 1 => BandDataType::UInt8,
+ 2 => BandDataType::UInt16,
+ 3 => BandDataType::Int16,
+ 4 => BandDataType::UInt32,
+ 5 => BandDataType::Int32,
+ 6 => BandDataType::Float32,
+ 7 => BandDataType::Float64,
+ 12 => BandDataType::UInt64,
+ 13 => BandDataType::Int64,
+ 14 => BandDataType::Int8,
_ => panic!(
"Unknown band data type: {}",
Review Comment:
Sure. I found that storage_type also need to be fixed, I have changed both storage_type() and data_type() to return Result<...>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types, don't panic in BandMetadataRef [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2797706521
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,22 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
-/// Only supports basic numeric types.
-/// In future versions, consider support for complex types used in
-/// radar and other wave-based data.
+/// Ordinals match GDALDataType for real-valued pixel types only.
+/// GDT_Unknown (0) and complex types (CInt16=8, CInt32=9, CFloat32=10, CFloat64=11)
+/// are intentionally omitted.
Review Comment:
We don't need `Other` variant for now since we decided to not enforce the values of `BandDataType` to match GDAL. I believe that it is a better design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types, don't panic in BandMetadataRef [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation merged PR #589:
URL: https://github.com/apache/sedona-db/pull/589
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2797683416
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,22 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
-/// Only supports basic numeric types.
-/// In future versions, consider support for complex types used in
-/// radar and other wave-based data.
+/// Ordinals match GDALDataType for real-valued pixel types only.
+/// GDT_Unknown (0) and complex types (CInt16=8, CInt32=9, CFloat32=10, CFloat64=11)
+/// are intentionally omitted.
#[repr(u16)]
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Copy)]
pub enum BandDataType {
Review Comment:
The values here were happened to be the same as GDAL band data types. We'll have functions for converting between then rather than implicitly relying on it. https://github.com/Kontinuation/sedona-db/blob/0e73e17a79332cf7e58b9881084d86dd3f3531c1/rust/sedona-raster-gdal/src/gdal_common.rs#L50-L88
I decided to simply define the values without GDAL awareness just as before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2793657213
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,23 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
+/// Values align with GDALDataType ordinals for non-complex types (excluding Unknown).
/// Only supports basic numeric types.
/// In future versions, consider support for complex types used in
/// radar and other wave-based data.
Review Comment:
Will improve the comment to make it more clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2793637360
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,23 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
+/// Values align with GDALDataType ordinals for non-complex types (excluding Unknown).
/// Only supports basic numeric types.
/// In future versions, consider support for complex types used in
/// radar and other wave-based data.
Review Comment:
The comment states ordinals align with GDAL's non-complex types 'excluding Unknown', but the mapping is incomplete. GDAL ordinals 8-11 (CInt16, CInt32, CFloat32, CFloat64) are skipped without explanation. Consider clarifying that this enum only includes real-valued types and omits both GDT_Unknown (0) and all complex types (8-11).
```suggestion
/// Discriminant values 1–7 align with `GDALDataType` ordinals for real-valued
/// (non-complex) types. `GDT_Unknown` (0) and all complex types (8–11) are
/// intentionally omitted. Values 12–14 are Sedona-specific extensions that do
/// not correspond to any `GDALDataType` value.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2794931658
##########
rust/sedona-raster/src/array.rs:
##########
@@ -146,13 +146,16 @@ impl<'a> BandMetadataRef for BandMetadataRefImpl<'a> {
fn data_type(&self) -> BandDataType {
match self.datatype_array.value(self.band_index) {
- 0 => BandDataType::UInt8,
- 1 => BandDataType::UInt16,
- 2 => BandDataType::Int16,
- 3 => BandDataType::UInt32,
- 4 => BandDataType::Int32,
- 5 => BandDataType::Float32,
- 6 => BandDataType::Float64,
+ 1 => BandDataType::UInt8,
+ 2 => BandDataType::UInt16,
+ 3 => BandDataType::Int16,
+ 4 => BandDataType::UInt32,
+ 5 => BandDataType::Int32,
+ 6 => BandDataType::Float32,
+ 7 => BandDataType::Float64,
+ 12 => BandDataType::UInt64,
+ 13 => BandDataType::Int64,
+ 14 => BandDataType::Int8,
_ => panic!(
"Unknown band data type: {}",
Review Comment:
Should this return an error or is this validated before we get here? (i.e., do we risk a panic for a corrupted raster array?)
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,22 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
-/// Only supports basic numeric types.
-/// In future versions, consider support for complex types used in
-/// radar and other wave-based data.
+/// Ordinals match GDALDataType for real-valued pixel types only.
+/// GDT_Unknown (0) and complex types (CInt16=8, CInt32=9, CFloat32=10, CFloat64=11)
+/// are intentionally omitted.
#[repr(u16)]
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Copy)]
pub enum BandDataType {
Review Comment:
This is probably more flexible without the `#[repr(u16)]` since at some point we may need a variant to handle other types of data (e.g., generic fixed size). It's great to align our representation to GDAL's but it is probably better scoped as `try_to_gdal_band_data_type() -> u16` and `try_from_gdal_band_data_type(gdal_type: u16)`.
##########
rust/sedona-schema/src/raster.rs:
##########
@@ -88,19 +88,22 @@ impl RasterSchema {
/// Band data type enumeration for raster bands.
///
-/// Only supports basic numeric types.
-/// In future versions, consider support for complex types used in
-/// radar and other wave-based data.
+/// Ordinals match GDALDataType for real-valued pixel types only.
+/// GDT_Unknown (0) and complex types (CInt16=8, CInt32=9, CFloat32=10, CFloat64=11)
+/// are intentionally omitted.
Review Comment:
You could handle these with an `Other(usize)` variant (whose internal representation is opaque bytes).
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -605,6 +605,7 @@ mod tests {
// Test all BandDataType variants
let test_cases = vec![
(BandDataType::UInt8, vec![1u8, 2u8, 3u8, 4u8]),
+ (BandDataType::Int8, vec![255u8, 254u8, 253u8, 252u8]), // -1, -2, -3, -4 as i8
Review Comment:
If possible this may be a good time to add test for a corrupted band identifier to make sure it doesn't panic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-schema): Add Int8, UInt64, Int64 band data types and align ordinals with GDAL [sedona-db]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #589:
URL: https://github.com/apache/sedona-db/pull/589#discussion_r2797684567
##########
rust/sedona-raster/src/builder.rs:
##########
@@ -605,6 +605,7 @@ mod tests {
// Test all BandDataType variants
let test_cases = vec![
(BandDataType::UInt8, vec![1u8, 2u8, 3u8, 4u8]),
+ (BandDataType::Int8, vec![255u8, 254u8, 253u8, 252u8]), // -1, -2, -3, -4 as i8
Review Comment:
Added `test_invalid_band_metadata_returns_err` for testing this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org