drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hsuanyi <...@git.apache.org>
Subject [GitHub] drill pull request: DRILL-4525: Allow SqlBetweenOperator to accept...
Date Thu, 24 Mar 2016 16:47:28 GMT
Github user hsuanyi commented on the pull request:

    https://github.com/apache/drill/pull/439#issuecomment-200919168
  
    In addition to @jinfengni's point:
    @sudheeshkatkam did a proposal to add a new SqlAvgAggFunction (the inference mechanism
of the original is not ideal for Drill). It was declined by Calcite's community, and the argument
was adding unnecessary duplicates to the code. I think this argument is fair. Otherwise, there
might be tens of variants for a single operator.
    
    Regarding adding a new converlet:
    Initially, I though having three wrappers (operator, function, agg function) are sufficient
for all scenarios. However, at one method during conversion, the type SqlBetweenOperator is
being expected. Thus, I decided to add a new wrapper which extends SqlBetweenOperator, so
we do not need to do anything in conversion.
    
    Certainly, we might be able to attain by not adding a new wrapper but adding a new converlet.
I would not prefer this approach since 
    1. I tried not to spread out the code complexity (introduced by inference and operand
type check) to too many places. 
    2. Adding our own converlet also introduces code complexity. And it probably is more complicated
than a wrapper.


---
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