Features with different types but same value are stored together

[Algunenano]

If I try to add 2 key-value pairs in a layer and both values have different types but same value (for example 3 and 3.0) both keys are added but only the first of the values is added, and the second key will point to the first one index.

This is an problem as, when storing the values in protozero, we store not only the value but the type (Value_Encoding) too, so when extracting the data instead of getting 'int(3)' and 'float(3.0)' we'll get have 'int(3)' for both. It's even more troublesome with bools as storing 'boolean(false)' and 'int(0)' will lead to both values becoming 'false' when extracting.

The issue is that the 'mapnik::value' comparison is done purely by value (references here and here so 0.0 is equal to 0 and equal to false. Small sample with the comparison:

// g++ a.cpp -std=c++11 $(pkg-config --cflags --libs icu-io) $(mapnik-config --libs) $(mapnik-config --includes)
#include <mapnik/value.hpp>
#include <cstdio>
#include <unordered_map>

int main(int argc, char** argv)
{
    std::unordered_map<mapnik::value, unsigned> m;
    mapnik::value a(false);
    mapnik::value b(0);
    m.emplace(a, 0);

    if (m.find(b) != m.end())
    {
        printf("Found\n");
    }
    else
    {
        printf("Not found\n");
    }
}

$ g++ a.cpp -std=c++11 $(pkg-config --cflags --libs icu-io) $(mapnik-config --libs) $(mapnik-config --includes)
$ ./a.out 
Found

2 questions:

• Is this the intended behaviour? As mentioned above it's causing us issues with values randomly switching from int/float to boolean and viceversa.
• I understand this should be addressed here and not in mapnik, since in mapnik you expect mapnik::value(1) to be equal to mapnik::value(1.0). Am I correct? I have a working solution which involves using std::unordered_map<std::pair<Value_Encoding, mapnik::value>, unsigned> as container instead of std::unordered_map<mapnik::value, unsigned> so I will create a PR if this behaviour isn't intended and should be fixed here.

[flippmoke]

This definitely seems like a bug -- if you create a PR please make a unit test to show that it is fixed.

[artemp]

@Algunenano - storing Value_Encoding looks like a good approach here as mapnik::value is not the same as protozero value.
/cc @flippmoke

[Algunenano]

I tried the approach that I was talking about (using a std::pair<Value_Encoding, mapnik::value>) but the code ended up being overcomplicated with multiple visitors. I decided instead to go for just declaring custom hash and compare functions instead taking into account the associated Value_Encoding type.

[Algunenano]

Ey, do you need anything apart from https://github.com/mapbox/mapnik-vector-tile/pull/281 for this issue? Let me know if you need any change to the PR to get this fixed.