[ 
https://issues.apache.org/jira/browse/DRILL-4726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15468221#comment-15468221
 ] 

ASF GitHub Bot commented on DRILL-4726:
---------------------------------------

Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/574#discussion_r77697563
  
    --- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java
 ---
    @@ -218,4 +302,141 @@ private void 
registerOperatorsWithoutInference(DrillOperatorTable operatorTable)
           }
         }
       }
    +
    +  /**
    +   * Function registry holder. Stores function implementations by jar 
name, function name.
    +   */
    +  private class GenericRegistryHolder<T, U> {
    +    private final ReadWriteLock readWriteLock = new 
ReentrantReadWriteLock();
    +    private final AutoCloseableLock readLock = new 
AutoCloseableLock(readWriteLock.readLock());
    +    private final AutoCloseableLock writeLock = new 
AutoCloseableLock(readWriteLock.writeLock());
    +
    +    // jar name, Map<function name, List<signature>
    +    private final Map<T, Map<T, List<T>>> jars;
    +
    +    // function name, Map<signature, function holder>
    +    private final Map<T, Map<T, U>> functions;
    +
    +    public GenericRegistryHolder() {
    +      this.functions = Maps.newHashMap();
    +      this.jars = Maps.newHashMap();
    +    }
    +
    +    public void addJar(T jName, Map<T, Pair<T, U>> sNameMap) {
    +      try (AutoCloseableLock lock = writeLock.open()) {
    +        Map<T, List<T>> map = jars.get(jName);
    +        if (map != null) {
    +          removeAllByJar(jName);
    +        }
    +        map = Maps.newHashMap();
    +        jars.put(jName, map);
    +
    +        for (Entry<T, Pair<T, U>> entry : sNameMap.entrySet()) {
    +          T sName = entry.getKey();
    +          Pair<T, U> pair = entry.getValue();
    +          addFunction(jName, pair.getKey(), sName, pair.getValue());
    --- End diff --
    
    Rather expensive. We have the function map right in our hands. Yet, on 
every add function, we look it up again.
    
    Perhaps the code would be simpler if we declared intermediate classes such 
as one that is the method registry for a function, rather than using generic 
classes. That way, your method registry class could do the work to register 
methods concisely rather than trying to do it generically as here.
    
    Also, having smaller classes makes unit testing easier. (This class is unit 
tested, right? Actually, it can't be: the class is private so not reachable 
from a unit test...) This raises a question: how will we test the detailed 
logic here if we don't have unit tests? Using system level tests that send SQL 
to Drill?


> Dynamic UDFs support
> --------------------
>
>                 Key: DRILL-4726
>                 URL: https://issues.apache.org/jira/browse/DRILL-4726
>             Project: Apache Drill
>          Issue Type: New Feature
>    Affects Versions: 1.6.0
>            Reporter: Arina Ielchiieva
>            Assignee: Arina Ielchiieva
>             Fix For: Future
>
>
> Allow register UDFs without  restart of Drillbits.
> Design is described in document below:
> https://docs.google.com/document/d/1FfyJtWae5TLuyheHCfldYUpCdeIezR2RlNsrOTYyAB4/edit?usp=sharing
>  



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to