Stop leaking Avro objects from parser #12828
Conversation
The Avro parsing code leaks some "object" representations. We need to convert them into Maps/Lists so that other code can understand and expect good things. Previously, these objects were handled with .toString(), but that's not a good contract in terms of how to work with objects.
| @@ -173,6 +175,20 @@ private Object transformValue(final Object field) | |||
| } else { | |||
| return ((GenericFixed) field).bytes(); | |||
| } | |||
| } else if (field instanceof Map) { | |||
There was a problem hiding this comment.
I wonder if this should use like if (avroJsonProvider.isArray(field)) ... andif (avroJsonProvider.isMap(field)) ...
There are also methods like avroJsonProvider.length, avroJsonProvider.getArrayIndex, avroJsonProvider.getPropertyKeys, avroJsonProvider.getMapValue etc. which could be used to extract stuff, though the array one doesn't seem that useful since it just handles List anyway.
| @@ -164,7 +166,7 @@ private Object transformValue(final Object field) | |||
| } else if (field instanceof Utf8) { | |||
| return field.toString(); | |||
| } else if (field instanceof List) { | |||
| return ((List<?>) field).stream().filter(Objects::nonNull).collect(Collectors.toList()); | |||
| return ((List<?>) field).stream().filter(Objects::nonNull).map(this::transformValue).collect(Collectors.toList()); | |||
There was a problem hiding this comment.
this could possibly generate new output for a list which has ByteBuffer in it. But I think if anyone is using this then their data might look absurd since doing a toString on ByteBuffer doesn't return the backed bytes.
| @@ -0,0 +1 @@ | |||
| This extension enables parsing of Avro data | |||
There was a problem hiding this comment.
heh, this readme is causing the license check stuff to fail because it is missing a license header. Suggest deleting since its basically empty, or adding
<!--
~ Licensed to the Apache Software Foundation (ASF) under one
~ or more contributor license agreements. See the NOTICE file
~ distributed with this work for additional information
~ regarding copyright ownership. The ASF licenses this file
~ to you under the Apache License, Version 2.0 (the
~ "License"); you may not use this file except in compliance
~ with the License. You may obtain a copy of the License at
~
~ http://www.apache.org/licenses/LICENSE-2.0
~
~ Unless required by applicable law or agreed to in writing,
~ software distributed under the License is distributed on an
~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
~ KIND, either express or implied. See the License for the
~ specific language governing permissions and limitations
~ under the License.
-->
``` to make the checks happy
The Avro parsing code leaks some "object" representations. We need to convert them into Maps/Lists so that other code can understand and expect good things.
Previously, these objects were handled with .toString(), but that's not a good contract in terms of how to work with objects. That said, this does potentially introduce a chance for "backwards incompatibility" because anybody who happened to actually have these objects "leak" into their ingestion would have ended up with these objects being stored as a String column with a value as the .toString(). When they update to this new code, they will still have a String column, but it will be the String returned from
Map.toString()instead of theGenericRecord.toString. So, the actual content would be slightly different.Relying on this behavior is... questionable at best. If someone is actually relying on this, then they can have the option of grabbing the avro extension from a previous version and continuing to use that instead of using this updated version. In terms of the behavior expected out-of-the-box from the code, the behavior that this change creates is significantly better. So, given that depending on the previous behavior is likely to be low value anyway and that there is a meaningful workaround in that you can choose to use the old extension jar instead, we should accept the compatibility change and make forward progress.
This PR has: