james-willis commented on code in PR #749:
URL: https://github.com/apache/sedona-db/pull/749#discussion_r3239185874
##########
rust/sedona-raster/src/array.rs:
##########
@@ -230,11 +233,16 @@ impl<'a> RasterRef for RasterRefImpl<'a> {
// Only the canonical identity view (null view row) is written today.
// A non-null view row would require the view → byte-stride composition
- // path that is deferred to a follow-up; reject it here so callers see
- // a clean "no band" rather than a panic.
- if !self.band_view_list.is_null(band_row) {
- return None;
- }
+ // path, which is not yet implemented. Surface it loudly here rather
+ // than silently returning None, so callers see a clear invariant
+ // violation instead of an out-of-range-looking miss.
+ assert!(
+ self.band_view_list.is_null(band_row),
+ "{}",
+ sedona_common::sedona_internal_datafusion_err!(
+ "non-null view row at band {band_row}: view composition is not
yet implemented"
+ )
+ );
Review Comment:
It was intentional; this method returns an Optional not a Result.
Though now I'm wondering if it should be optional. the original idea was to
return none for band numbers that don't exist but maybe the old behavior to
return a Err was better.
##########
rust/sedona-raster/src/array.rs:
##########
@@ -230,11 +233,16 @@ impl<'a> RasterRef for RasterRefImpl<'a> {
// Only the canonical identity view (null view row) is written today.
// A non-null view row would require the view → byte-stride composition
- // path that is deferred to a follow-up; reject it here so callers see
- // a clean "no band" rather than a panic.
- if !self.band_view_list.is_null(band_row) {
- return None;
- }
+ // path, which is not yet implemented. Surface it loudly here rather
+ // than silently returning None, so callers see a clear invariant
+ // violation instead of an out-of-range-looking miss.
+ assert!(
+ self.band_view_list.is_null(band_row),
+ "{}",
+ sedona_common::sedona_internal_datafusion_err!(
+ "non-null view row at band {band_row}: view composition is not
yet implemented"
+ )
+ );
Review Comment:
It was intentional; this method returns an Option not a Result.
Though now I'm wondering if it should be optional. the original idea was to
return none for band numbers that don't exist but maybe the old behavior to
return a Err was better.
--
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]
