soveran / ohm

Object-Hash Mapping for Redis
http://ohm.keyvalue.org
MIT License
1.4k stars 167 forks source link

Index keys for floats of form "n.0" are truncated #211

Closed slowernet closed 8 years ago

slowernet commented 8 years ago

If an indexed attribute is assigned a real value with 0 fractional part (eg. 3.0), the attribute is stored in the hash with that format, as expected, but the index key is truncated to an integer (Model:indices:attribute:3). I'd argue that the index key should string match the stored attribute value, as it does for all other real numbers.

IMO this is unfortunate behavior by lua's tostring, here:

local key = model.name .. ":indices:" .. field .. ":" .. tostring(val)

Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
> print(tostring(3.1))
3.1
> print(tostring(3.0))
3

Thank you.

slowernet commented 8 years ago

I experimented with a bunch of workarounds, including lua's string.format, but it's difficult to figure out how to write the format string (and you also have to detect when the attribute value is a float in the first place).

The best solution I have found is using the string literal delimiter [[]] instead of tostring(). But infuriatingly this fails in instances when the value itself contains a right square bracket.

Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
> print(tostring(3.0))
3
> print(3.0)
3
> print([[3.0]])
3.0
> print([[[]])
[
> print([[]]])
stdin:1: ')' expected near ']'

There's probably a way to escape this, will keep experimenting.

soveran commented 8 years ago

Great catch! I think this commit fixes the issue, can you confirm?

slowernet commented 8 years ago

If there's no performance concern about doing it in Ruby, that seems like a good solution.

I did learn that it's possible to use a variant of the double square bracket delimiter in Lua: adding equals signs between the opening brackets (eg. [====[) means that it won't terminate until the matching close bracket is found (]====]), similar to heredoc (<<-EOS ... EOS) in Ruby. That would seem to be a safe alternative if you wanted to do the conversion in Lua, though I have not written tests.

soveran commented 8 years ago

I run some benchmarks and if I remove the tostring(val) in Lua and add the .to_s in Ruby, I get about the same performance (actually slightly better, but I would blame the inaccuracy of the benchmarks for that tiny difference). Having a contract that the Lua script must receive strings looks like the right approach, given the fact that tostring is unreliable.

slowernet commented 8 years ago

Thanks for the fix!