"Invalid time format" Error thrown

christopherpetro commented 9 years ago

We have a service which has been running fine for a while now. The entire server is repeatedly going down today because the vertica driver is throwing an Error. Stack trace is below.

I took a look at the source, and this file is riddled with thrown Errors. Shouldn't these all be caught in query.js and returned to the callback? Taking down the entire app because an unexpected data format was received seems a bit drastic.

The value in the buffer which caused this was in the format "10:34:01.16". Apparently the time values are not limited to second precision. I patched our code for now to handle this by just discarding the fractional part, but I think this needs a more thoughtful approach.

node_modules/vertica/lib/types.js:108
    throw new Error('Invalid time format!');
          ^
Error: Invalid time format!
    at Function.VerticaTime.fromStringBuffer (node_modules/vertica/lib/types.js:108:11)
    at Field.stringDecoders.time [as decoder] (node_modules/vertica/lib/types.js:259:24)
    at Query.onDataRow (node_modules/vertica/lib/query.js:85:51)
    at Connection.emit (events.js:95:17)
    at Connection._onData (node_modules/vertica/lib/connection.js:350:14)
    at Socket.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:764:14)
    at Socket.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:426:10)
    at emitReadable (_stream_readable.js:422:5)

wvanbergen commented 9 years ago

In this case, I don't think this should be returned by the callback because this looks like a bug to me. We should be able to parse any timestamp that Vertica sends back.

Any chance you can figure out what vertica is sending that makes it fail?

christopherpetro commented 9 years ago

See above. The string was "10:34:01.16". The vertica docs say TIME has microsecond resolution, but I don't know how that works out at the protocol level.

wvanbergen commented 9 years ago

This can be fixed by fixing the regular expression that parses Time values (https://github.com/wvanbergen/node-vertica/blob/master/src/types.coffee#L74) and Timestamp values (https://github.com/wvanbergen/node-vertica/blob/master/src/types.coffee#L92)

I don't have time for this ATM, feel feel free to open a PR to fix this and I will merge.

christopherpetro commented 9 years ago

That's how I hotpatched it as a stop-gap measure. Right now I'm just discarding the sub-second data, but it seems like it should be preserved. Do you have any idea if VerticaTime will work correctly with a non-integer seconds value? I haven't gotten that far into the code yet.

wvanbergen commented 9 years ago

The VerticaTime type is only used for two things:

To return to the user if the resultset includes a time column. This can be fixed by either making the seconds field a float instead of an integer, or add a separate microseconds field. I have no strong preference
A user can send a VerticaTime value to quote to properly include a time value in a SQL query: https://github.com/wvanbergen/node-vertica/blob/master/src/types.coffee#L70. You may want to make sure this gets a fix as well.

Just add some tests to show it's working as expected and I will merge :+1:

wvanbergen / node-vertica

"Invalid time format" Error thrown #44