zeroshade commented on code in PR #38028:
URL: https://github.com/apache/arrow/pull/38028#discussion_r1347530471
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Float16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx).Float32()), nil
Review Comment:
should a float16 return a `float64`? or should it return `float16.Num`? I
would lean towards the latter
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Float16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx).Float32()), nil
case *array.Float32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx)), nil
Review Comment:
why convert to float64 rather than just return the `float32`?
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
Review Comment:
because `IsNull` is in the `arrow.Array` interface, you can probably factor
this out and just do `if arr.IsNull(idx) { return nil, nil }` before the
typeswitch both to simplify the code and to create a fast path for nulls to
avoid the type switch entirely in that case.
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Float16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx).Float32()), nil
case *array.Float32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx)), nil
case *array.Float64:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
+ case *array.Decimal128:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ dt, ok := arr.DataType().(*arrow.Decimal128Type)
+ if !ok {
+ return nil, fmt.Errorf("datatype %T not matching
decimal128", arr.DataType())
+ }
+
+ return c.Value(idx).ToFloat64(dt.Scale), nil
+ case *array.Decimal256:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ dt, ok := arr.DataType().(*arrow.Decimal256Type)
+ if !ok {
+ return nil, fmt.Errorf("datatype %T not matching
decimal256", arr.DataType())
+ }
+
+ return c.Value(idx).ToFloat64(dt.Scale), nil
case *array.Int8:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return int64(c.Value(idx)), nil
Review Comment:
i know that this is the existing code, but since we're in this discussion,
does it make sense to upcast to int64 rather than letting it stay int8?
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Float16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx).Float32()), nil
case *array.Float32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx)), nil
case *array.Float64:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
+ case *array.Decimal128:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ dt, ok := arr.DataType().(*arrow.Decimal128Type)
+ if !ok {
+ return nil, fmt.Errorf("datatype %T not matching
decimal128", arr.DataType())
+ }
+
+ return c.Value(idx).ToFloat64(dt.Scale), nil
+ case *array.Decimal256:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ dt, ok := arr.DataType().(*arrow.Decimal256Type)
+ if !ok {
+ return nil, fmt.Errorf("datatype %T not matching
decimal256", arr.DataType())
+ }
+
+ return c.Value(idx).ToFloat64(dt.Scale), nil
case *array.Int8:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return int64(c.Value(idx)), nil
case *array.Int16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return int64(c.Value(idx)), nil
case *array.Int32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return int64(c.Value(idx)), nil
case *array.Int64:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ return c.Value(idx), nil
+ case *array.Binary:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.String:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Time32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
dt, ok := arr.DataType().(*arrow.Time32Type)
if !ok {
return nil, fmt.Errorf("datatype %T not matching
time32", arr.DataType())
}
Review Comment:
same statement as above, there's no need to confirm the time32type here, it
can be safely assumed
##########
go/arrow/flight/flightsql/driver/utils.go:
##########
@@ -60,44 +60,135 @@ func (g grpcCredentials) RequireTransportSecurity() bool {
func fromArrowType(arr arrow.Array, idx int) (interface{}, error) {
switch c := arr.(type) {
case *array.Boolean:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
case *array.Float16:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx).Float32()), nil
case *array.Float32:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return float64(c.Value(idx)), nil
case *array.Float64:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
return c.Value(idx), nil
+ case *array.Decimal128:
+ if c.IsNull(idx) {
+ return nil, nil
+ }
+
+ dt, ok := arr.DataType().(*arrow.Decimal128Type)
+ if !ok {
+ return nil, fmt.Errorf("datatype %T not matching
decimal128", arr.DataType())
+ }
Review Comment:
this check is unnecessary, there should never be a case where the datatype
for a `Decimal128` is not a `Decimal128Type`
--
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]
