drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From paul-rogers <...@git.apache.org>
Subject [GitHub] drill pull request #819: DRILL-5419: Calculate return string length for lite...
Date Fri, 28 Apr 2017 01:29:31 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/819#discussion_r113835436
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillConcatFuncHolder.java
---
    @@ -0,0 +1,62 @@
    +/*
    + * 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.
    + */
    +package org.apache.drill.exec.expr.fn;
    +
    +import org.apache.drill.common.expression.LogicalExpression;
    +import org.apache.drill.common.types.TypeProtos;
    +import org.apache.drill.common.types.Types;
    +
    +import java.util.List;
    +
    +/**
    + * Function holder for functions with function scope set as
    + * {@link org.apache.drill.exec.expr.annotations.FunctionTemplate.FunctionScope#CONCAT}.
    + */
    +public class DrillConcatFuncHolder extends DrillSimpleFuncHolder {
    --- End diff --
    
    As Jinfeng noted, we are extending a concept meant for one purpose and using it for another.
We may not be stretching it to the breaking point yet, but I can see that coming.
    
    What we really have is the idea of a function definition (what we call a "holder.") That
definition has many aspects: signature, type inference, etc. We are using a single class with
all properties and using inheritance to share properties. (Here, we are using inheritance
for any concat-style function.)
    
    But, what happens for a function that acts like concat in one area, but like something
else in another? We have to copy & paste code to create a mixture class.
    
    Better would be to hold each property as a separate class. That is, decorate the function
definition with its types, with its cast rules, with its type inference rules, and so on,
each expressed as a class.
    
    By this:
    ```
    public interface TypeInference {
      MajorType returnType();
    }
    public interface CastInference {
      Something inferCast();
    }
    
    Then, create instances that implement each rule set. Finally, create function rules that
are collection of property rules:
    
    FunctionDefn = new FunctionDefnBuilder()
      .returnType( concatReturnType )
      .cast( somethingCastRules )
      .name( "org.apache.drill.some.package.MyFunc" )
      .build();
    ```
    The important thing isn't the specific classes. These are just by way of illustrating
the general idea; which can (and probably would) be adjusted to fit.
    
    Again, we may not need this quite yet. But, once we see copy & paste of method bodies,
we'll know we need some refactoring.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message