Add feature: listKeys Scalar Transformation Function #13581
Conversation
| @@ -0,0 +1,67 @@ | |||
| package org.apache.pinot.common.function.scalar; | |||
| import java.util.Map; | ||
|
|
||
| public class MapFunctions { | ||
| private MapFunctions() { |
There was a problem hiding this comment.
Please apply the Pinot Style and reformat the changes
There was a problem hiding this comment.
Attempted with new diff, please ping if it still isn't resolved.
| // CHECKSTYLE:OFF | ||
| // @formatter:off |
There was a problem hiding this comment.
(minor) No need to turn off them, same for testNull()
| public void testRegMapSameTypes() | ||
| throws JsonProcessingException { | ||
|
|
||
| // CHECKSTYLE:OFF |
There was a problem hiding this comment.
(minor) No need to turn off CHECKSTYLE, same for other places
| * @param jsonString | ||
| * @return converted Java Map from JSON String | ||
| */ | ||
| public static Map stringToMap(String jsonString) { |
There was a problem hiding this comment.
You should be able to use JsonUtils.jsonNodeToMap()
There was a problem hiding this comment.
Redid implementation with a LinkedHashMap, is this function still usable? I further noticed that jsonUtils' jsonNodeToMap takes in a JSON Object whereas this new implementation is string based, do you have any thoughts on to still use jsonNodeToMap() or maybe add in a function there that takes string-based input?
There was a problem hiding this comment.
Any specific reason why you need a LinkedHashMap?
JsonUtils.stringToObject() can be used in order to deserialize to a custom object
|
|
||
| //add map keys to an object list | ||
| List<Object> mapList = new ArrayList<>(convertedMap.size()); | ||
| mapList.addAll(convertedMap.keySet()); |
There was a problem hiding this comment.
is this convertedMap.keySet() order guaranteed? if we ingest data with the udf, will the output consistent from the all partitions?
There was a problem hiding this comment.
No, addressed with LinkedHashMap change in new commit
| import java.util.Map; | ||
| import org.apache.pinot.spi.annotations.ScalarFunction; | ||
|
|
||
| public class MapFunctions { |
There was a problem hiding this comment.
Please apply Pinot Style and reformat the changes
| public class MapFunctions { | ||
| private MapFunctions() { } | ||
|
|
||
| public static final ObjectMapper oM = new ObjectMapper(); |
There was a problem hiding this comment.
You should be able to use JsonUtils for JSON ser-de
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13581 +/- ##
============================================
+ Coverage 61.75% 61.96% +0.21%
+ Complexity 207 198 -9
============================================
Files 2436 2554 +118
Lines 133233 140532 +7299
Branches 20636 21857 +1221
============================================
+ Hits 82274 87083 +4809
- Misses 44911 46824 +1913
- Partials 6048 6625 +577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| public static String[] listMapEntries(String jsonString) { | ||
| // handle empty string case | ||
| if (jsonString == null) { |
There was a problem hiding this comment.
If you want this function to be able to take null values as input, you'll need to add nullableParameters = true to the @ScalarFunction annotation. By default, scalar functions are assumed to be null "intolerant", so if any argument to the function is null, the value null will be returned without even invoking the function (see FunctionInvoker). Same goes for the other scalar functions added here.
| * @param key | ||
| * @return corresponding value for given key from JSON map in data | ||
| */ | ||
| public static Object getValueFromKey(Object object, Object key) { |
There was a problem hiding this comment.
This shouldn't be a public method in a non-testing class if it's only being used in unit tests.
add listKeys() scalar transformation function and unit tests under new mapfunctions class. takes in JSON map input and outputs string array of all keys in map.