DRILL-4456: Add Hive translate UDF #1527
Conversation
|
@vdiravka could you please review. |
|
@vvysotskyi The PR with two commits makes sense if that tickets are interdependent. |
vvysotskyi
force-pushed
the
DRILL-4456
branch
from
2af1cf0
to
0109c8b
Compare
vvysotskyi
changed the title
| * whether specified function names should be replaced | ||
| * with the names from the map. | ||
| */ | ||
| private String[] renameUDF(String[] namesToCheck) { |
There was a problem hiding this comment.
Please add @param and @return.
| /** | ||
| * Map for renaming Hive UDFs whose names satisfy the predicate in the key by names from the map value. | ||
| */ | ||
| private static final Map<Predicate<String[]>, String[]> FUNCTION_REPLACE_MAP = ImmutableMap.<Predicate<String[]>, String[]> builder() |
There was a problem hiding this comment.
Please add better description.
There was a problem hiding this comment.
@vvysotskyi Try to simplify the logic for FUNCTION_REPLACE_MAP. Possibly try to get rid from String[] (you can add all function names to this Map), so in result it can be similar to
https://github.com/apache/drill/blob/master/logical/src/main/java/org/apache/drill/common/expression/fn/FunctionReplacementUtils.java#L42
There was a problem hiding this comment.
Thanks, reworded a description and simplified the map to use target function names as keys and predicates for checking as values.
There was a problem hiding this comment.
@arina-ielchiieva, @vdiravka, Thanks for the review, I have addressed CR comments, could you please take a look again?
| /** | ||
| * Map for renaming Hive UDFs whose names satisfy the predicate in the key by names from the map value. | ||
| */ | ||
| private static final Map<Predicate<String[]>, String[]> FUNCTION_REPLACE_MAP = ImmutableMap.<Predicate<String[]>, String[]> builder() |
There was a problem hiding this comment.
Thanks, reworded a description and simplified the map to use target function names as keys and predicates for checking as values.
| * whether specified function names should be replaced | ||
| * with the names from the map. | ||
| */ | ||
| private String[] renameUDF(String[] namesToCheck) { |
vvysotskyi
force-pushed
the
DRILL-4456
branch
4 times, most recently
from
d8fab54
to
86b2656
Compare
vvysotskyi
changed the title
vvysotskyi
force-pushed
the
DRILL-4456
branch
2 times, most recently
from
1c0a2d8
to
3f366b1
Compare
| @@ -102,15 +116,15 @@ public void register(DrillOperatorTable operatorTable) { | |||
| } | |||
| } | |||
|
|
|||
| private <C,I> void register(Class<? extends I> clazz, ArrayListMultimap<String,Class<? extends I>> methods) { | |||
| private <I> void register(Class<? extends I> clazz, ArrayListMultimap<String, Class<? extends I>> methods) { | |||
There was a problem hiding this comment.
If it's not required to have separate renameUDF(names) method, then it's possible to simplify body of this method:
private <C, I> void register(Class<? extends I> clazz, ArrayListMultimap<String, Class<? extends I>> methods) {
Description desc = clazz.getAnnotation(Description.class);
Stream<String> names;
if (desc != null) {
names = Stream.of(desc.name().split(",")).map(String::trim);
} else {
names = Stream.of(clazz).map(Class::getName)
.map(name -> name.replace('.', '_'));
}
names.map(String::toLowerCase)
.map(funName -> FUNCTION_REPLACE_MAP.getOrDefault(funName, funName))
.forEach(udfFunName -> methods.put(udfFunName, clazz));
UDFType type = clazz.getAnnotation(UDFType.class);
if (type != null && !type.deterministic()) {
nonDeterministicUDFs.add(clazz);
}
}
3f366b1
to
58733cc
Compare
TRANSLATEfunction toTRANSLATE3during function registration.For problem description DRILL-4456.