Reject non-string keys

[suminb]

As far as I understand, JSON only allows string keys. However, the current implementation of MapColumnConverter allows arbitrary types as map keys which potentially cause issues for JsonConverter. This PR adds a safeguard that rejects non-string map keys.

[HenryCaiHaiying]

Does it guarantee that schema has Children? And what are each children schema representing?

[suminb]

Suppose we have an ORC schema like map<string,int> then the first child is a TypeDescription instance representing string type. Not sure if schema is guaranteed to have at least one child. We might want to test with a malformed ORC schema like map<>. It may or may not reach this point when such schema is given.

[HenryCaiHaiying]

Also make sure the passed-in 'schema' argument really represents the map schema

[suminb]

Also make sure the passed-in 'schema' argument really represents the map schema

It certainly wouldn't hurt to do so, but I feel like this is somewhat redundant as it is already ensured by VectorColumnFilter.createConverter(). Its very logic doesn't leave any room for other types to fall into MapColumnConverter.

    public static JsonConverter createConverter(TypeDescription schema) {
        switch (schema.getCategory()) {

        // ...

        case MAP:
            return new MapColumnConverter(schema);
        default:
            throw new IllegalArgumentException("Unhandled type " + schema);
        }
    }

If you still insist doing so, I would suggest to add some common type checking logic in JsonConverter interface or in VectorColumnFiller class so that other types could also benefit from it. Any thoughts on this?

[HenryCaiHaiying]

Should it be case insensitive comparison?

[suminb]

That’s a good point. Not sure if ORC schema is case sensitive. But it would be a good idea to perform case insensitive comparison here just to be safe.

[suminb]

I've tested what happens when ORC schema is defined with upper-case letters (e.g., map<INT,int>). When reached at MapColumnConverter, the schema shows the schema information as map<int,int>. Something in the middle automatically converts it to lower-case letters. However, I'll stick with a case-insensitive comparison, as you suggested, for extra safety.

[suminb]

Thanks for the review. I’ll come up with a better way to handle these concerns mentioned above and revise code accordingly.

[HenryCaiHaiying]

lgtm