Fix #217 double NaN regression

[franciscojunior]

Fix #217
After commit 854862a Npgsql wasn't adding quotes to float4 and float8 values anymore.
But according to documentation, NaN values need to be quoted.

This patch reverts quote handling of float4 and float8 values.

[franciscojunior]

@glenebob , would mind to review this patch and tell me if it is ok, please.
I just added quotes to float4 and float8 types so NaN values could be sent.

Thanks in advance.

[glenebob]

I think it would be better to quote selectively in the conversion function. It would be a tad more efficient, and of course you would usually not quote at all.

[glenebob]

I think you could call StringToTextText() if quoting is needed (after doing the conversion to string). That would insulate you from all the quoting complexity.

Now that I think about it, I don't think that makes quoting more efficient, but selective quoting is still a Very Good Thing here.

[franciscojunior]

Hi, @glenebob ! Thanks for reviewing.

I made some more changes based in your feedback. Now the quote is only added when handling the NaN case.

Please, have a look at the new patch and let me know if your think it is ok now.

Thanks in advance.

[glenebob]

Hi Francisco,

Did you test with an array of NaN, and with prepare? I think you need double quotes for arrays, and no quotes ever for extended. It's a little fuzzy now :)

Look at NpgsqlNativeTypeInfo.QuoteASCIIString(). That's the logic you need. Maybe you can make it internal and use that?

Or, look at StringToTextText(), specifically at how it gets a StringEncodingInfo object. That object contains a field for quote, and it will be ASCIIBytes.SingleQuote, ASCIIBytes.DoubleQuote, or zero, depending on what the backend expects. Look how it's used; if quote == 0, you just return the unquoted string (byte[]), otherwise put on the quote byte at each end.

-Glen

[franciscojunior]

Hi Francisco,

Hi, Glen!

Did you test with an array of NaN, and with prepare? I think you need double quotes for arrays, and no quotes ever for extended. It's a little fuzzy now :)

I didn't test with an array of NaN. This patch doesn't change prepared statements because in the current master, they are working ok. The bug only appears when using non prepared statements. But I'll add more tests to confirm that.

Look at NpgsqlNativeTypeInfo.QuoteASCIIString(). That's the logic you need. Maybe you can make it internal and use that?

Yes, I saw this method. I'll check it.

Off topic: I noticed that QuoteASCIIString uses another byte[] in order to add the quotes. I was thinking that in the future we could optimize that to not create a new buffer. Maybe we could defer this quoting until we are sending the data to the stream and then we would make (pseudo code):

stream.WriteByte("'");
stream.WriteBytes(byte[]);
stream.WriteByte("'");

Thanks for reviewing my patch!

[franciscojunior]

Hi, @glenebob ! You were right about array values. They were broken. But as before, only non prepared statements were affected. Now everything is ok.

I checked StringToTextText() method but I ended up just checking if the parameter was being used inside an array. NaN handling ended up being much simpler than text. :)

Please, have a look at it and let me know what you think.

Thanks in advance.

[roji]

@glenebob, @franciscojunior, this is currently marked for the 2.2 release, do you think we'll have time to complete it, or should I move it to 2.3?

[franciscojunior]

Em 27/07/2014 09:39, "Shay Rojansky" notifications@github.com escreveu:

@glenebob, @franciscojunior, this is currently marked for the 2.2 release, do you think we'll have to complete it, or should I move it to 2.3? This is a regression from 2.1. I'm just waiting a final review from
@glenebob. I think this patch is complete and as soon as Glen says it is OK
or ask for more modifications, I'll merge and backport it.

—
Reply to this email directly or view it on GitHub.

[glenebob]

Hi Francisco,

I'm finally able to look at this again. I had some ideas that were way beyond explaining briefly, so I grabbed your branch and added to it, and sent up a new PR, #306. Please have a look :)

-Glen

[glenebob]

Turns out this PR works great. PG does not use double quotes on NaN values in arrays.

[franciscojunior]

Great! I'll apply it to release-2.2.0 branch.