-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flattening Avro Maps with Utf8 keys #7258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! 👍
It looks like your PR has some style issues and is failing CI
[ERROR] /home/travis/build/apache/incubator-druid/extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/GenericAvroJsonProvider.java:142:77: '{' at column 77 should be on a new line. [LeftCurly]
[ERROR] /home/travis/build/apache/incubator-druid/extensions-core/avro-extensions/src/main/java/org/apache/druid/data/input/avro/GenericAvroJsonProvider.java:158:50: ',' is not followed by whitespace. [WhitespaceAfter]
Could you also please add a test case for this issue? It looks lgtm otherwise.
It would also appear that you have opened a duplicate PR, #7254. |
|
||
|
||
private Object transformAvroMapKey(String keyToTransform, Map avroMapObj) { | ||
Object key = avroMapObj.keySet().iterator().next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work if the map is empty?
It might be more straightforward to just do something like this in getMapValue
...
} else if (o instanceof Map) {
final Map theMap = (Map) o;
if (theMap.containsKey(s)) {
return theMap.get(s);
} else {
final Utf8 utf8Key = new Utf8(s);
return theMap.get(utf8Key);
}
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your suggestion is more elegant, I will commit it. Thks
Thanks @chrishardis for taking over the bug and committing it to master. |
I spend my morning trying to run a test case, but it's not so easy has using the io.confluent.kafka.serializers.KafkaAvroSerializer without a kafkaproducer is quite an hard job. I test it many times on real Confluent 5.1 Kafka Cluster with Schema Registry. |
I think this issue would exist for any type of Avro indexing; like if you could create an Avro formatted file with a column that is a map type with utf8 keys, you could test with the Hadoop indexing tests if that would be easier. That said, this change seems small and harmless enough that I think it would probably be ok without tests, so I will go ahead and approve. |
* #5884 Avro Maps can't be flattened in Kafka Indexing on master * correct typo and reduce code size
* apache#5884 Avro Maps can't be flattened in Kafka Indexing on master * correct typo and reduce code size
Fix issue #5884 by @varaga @jon-wei on master branch.
Successfully tested with Confluent 5.1.0 kafka and schema registry