jackc / pgtype

MIT License
319 stars 112 forks source link

DecodeBinary can't decode NaN #198

Closed ghstahl closed 1 year ago

ghstahl commented 1 year ago

Reference

Using TimescaleDB, but that should matter it's just postgres.

I am able to write NaN into postgres using this library.

github.com/jackc/pgtype v1.13.0
github.com/jackc/pgx/v4 v4.17.2

With PGAdmin I can query

SELECT * FROM public.point_series
"point_id"  "timestamp_utc" "timestamp_local"   "double_value"   
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:27+00"    "1970-01-01 00:00:27"   1.1         
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:28+00"    "1970-01-01 00:00:28"   NaN         
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:30+00"    "1970-01-01 00:00:30"   NaN         
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:31+00"    "1970-01-01 00:00:31"   NaN         
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:32+00"    "1970-01-01 00:00:32"   NaN         
"a0feae36-82bc-4cf8-bf63-68668af7e0a4"  "1970-01-01 00:00:33+00"    "1970-01-01 00:00:33"   NaN                     

I do a query and get back rows.

// var rows pgx.Rows
for rows.Next() {
err = rows.Scan(&pointID, &timestampUtc, &doubleValue, &geoJSON, &complexValue)
}

There is an error scanning a NaN into this doubleValue. it should just be math.NaN

func (dst *Numeric) DecodeBinary(ci *ConnInfo, src []byte) error {
...
*dst = Numeric{Status: Present, NaN: true}
...
}

then up the stack it detects is a NaN and turns it into an error

func (dst *Numeric) DecodeBinary(ci *pgtype.ConnInfo, src []byte) error {
...
    num := &pgtype.Numeric{}
    if err := num.DecodeBinary(ci, src); err != nil {
        return err
    }

    if num.NaN {
        return errors.New("cannot decode 'NaN'")
    }
...
}

What can I do to just get it sent back to me as

Numeric{Status: Present, NaN: true}

Its not an error to me.

ghstahl commented 1 year ago

I got a fix, probably just for my small use case, but this is what I did.

go.mod

replace github.com/jackc/pgtype => ../pgtype
replace github.com/shopspring/decimal => ../decimal

My personal copies of these libraries.

jackc.pgType

    if num.NaN {
        *dst = Numeric{Decimal: decimal.NewFromFloat(math.NaN()), Status: pgtype.Present}
        return nil
        //return errors.New("cannot decode 'NaN'")
    }

Shopspring.Decimal

func newFromFloat(val float64, bits uint64, flt *floatInfo) Decimal {
    //if math.IsNaN(val) || math.IsInf(val, 0) {
    //  panic(fmt.Sprintf("Cannot create a Decimal from %v", val))
    //}
...
}

I can't believe this even worked, but the end result is what I wanted. I have a grpc service and this is the output when I call it using BloomRPC

Request

{
  "point_id": "a0feae36-82bc-4cf8-bf63-68668af7e0a4",
  "start_time": {
    "seconds": 20,
    "nanos": 10
  },
  "end_time": {
    "seconds": 40,
    "nanos": 10
  },
  "latest": false,
  "pagination": {
    "limit": 100,
    "next_token": "",
    "page": 100
  }
}

Response

{
  "rows": [
    {
      "timestamp": {
        "seconds": "27",
        "nanos": 0
      },
      "value": {
        "float64_value": 1.1,
        "value": "float64_value"
      }
    },
    {
      "timestamp": {
        "seconds": "28",
        "nanos": 0
      },
      "value": {
        "float64_value": null,
        "value": "float64_value"
      }
    },
    {
      "timestamp": {
        "seconds": "30",
        "nanos": 0
      },
      "value": {
        "float64_value": null,
        "value": "float64_value"
      }
    },
    {
      "timestamp": {
        "seconds": "31",
        "nanos": 0
      },
      "value": {
        "float64_value": null,
        "value": "float64_value"
      }
    },
    {
      "timestamp": {
        "seconds": "32",
        "nanos": 0
      },
      "value": {
        "float64_value": null,
        "value": "float64_value"
      }
    },
    {
      "timestamp": {
        "seconds": "33",
        "nanos": 0
      },
      "value": {
        "float64_value": null,
        "value": "float64_value"
      }
    }
  ],
  "pagination": {
    "next_token": "-",
    "total_available": false,
    "total": "0",
    "no_more_data": true,
    "prev_token": ""
  }
}
ghstahl commented 1 year ago

Smallest Code Change Solution Suggestion Make pgtype NaN|INF|-INF aware;

type Status byte

const (
    Undefined Status = iota
    Null
    Present
    PresentNaN
    PresentInfinity
    PresentNegativeInfinity
)

Introduce a global setting to Not error out when NaN|INF|-INF is detected.

// TODO: put check here if we want to error on these types

if num.NaN {
  *dst = Numeric{Status: pgtype.PresentNaN}
  return nil
}
if num.InfinityModifier == pgtype.Infinity {
  *dst = Numeric{Status: pgtype.PresentInfinity}
  return nil
}
if num.InfinityModifier == pgtype.NegativeInfinity {
  *dst = Numeric{Status: pgtype.PresentNegativeInfinity}
  return nil
}

This way I can take care of what to do when I get them from the Row Scan (pgx is left alone here).

for rows.Next() {
  var pointID *string
  var timestampUtc time.Time
  var doubleValue shopspring.Numeric
  var geoJSON *string
  var complexValue map[string]interface{}

  err = rows.Scan(&pointID, &timestampUtc, &doubleValue, &geoJSON, &complexValue)

  switch doubleValue.Status {
  case pgtype.Present:
      doubleValue.AssignTo(&floatValue)
      rowValue.Value = &timeseries_api.RowValue_Float64Value{Float64Value: floatValue}
  case pgtype.PresentNaN:
      rowValue.Value = &timeseries_api.RowValue_Float64Value{Float64Value: math.NaN()}
  case pgtype.PresentInfinity:
      rowValue.Value = &timeseries_api.RowValue_Float64Value{Float64Value: math.Inf(1)}
  case pgtype.PresentNegativeInfinity:
      rowValue.Value = &timeseries_api.RowValue_Float64Value{Float64Value: math.Inf(-1)}
  default:
      foundDouble = false
  }
  ...
}
jackc commented 1 year ago

Just to be clear, this is only the shopspring/decimal integration, pgtype.Numeric supports NaN. The integration doesn't support NaN because shopspring/decimal doesn't support NaN.

I do not think it is a good idea to add NaN (or infinity) support to the standard integration. It would lock in a vestigial representation if shopspring/decimal later did add representation for those values. It is relatively simple to copy and customize the integration if you need support now in your own project.

ghstahl commented 1 year ago

Before we really close this out, the problem I have is that postgreSQL supports NaN, Infinity, -Infinity. The row.Scan just errors out when it hits these valid postgreSQL records. I would say that pgx/pgtype should support what postgreSQL supports and that has nothing to do with what shopspring supports. The pull request I had honored that without reliance on shopspring.

jackc commented 1 year ago

It does support NaN, Infinity, and -Infinity in pgtype.Numeric.

ghstahl commented 1 year ago

This code says otherwise. reference
As a result, the upstream caller rows.Scan() just stops and errors out.

jackc commented 1 year ago

See https://github.com/jackc/pgtype/blob/4baa50c409b72b4bc9c21258d716bfd2d772b5cc/numeric.go#L58. pgtype.Numeric supports NaN and Infinity. The shopspring/decimal integration doesn't because those are not valid values for shopspring/decimal.