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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
