Vincent, after deliberating with Greg Rahn, we concluded the second option (rewrite) would be preferable. Sorry to go back and forth on this.
Reasons: - We will likely soon add more functions that can make use of the same "accept in analysis + rewrite later" mechanism - The rewrite solution avoids code duplication - We can avoid generating many useless functions that just served the purpose of accepting multiple first arg types Please let me know if you need guidance to make progress. This rewrite solution might be more complicated to implement. Alex On Mon, May 15, 2017 at 8:19 PM, Vincent Tran <[email protected]> wrote: > That did the job. Thanks Bharath. > > On Mon, May 15, 2017 at 11:09 PM, Bharath Vissapragada < > [email protected]> wrote: > > > Added you to the relevant groups, could you check again please? > > > > On Mon, May 15, 2017 at 7:57 PM, Vincent Tran <[email protected]> > wrote: > > > > > My handle on ASF Jira is thundergun. > > > > > > On Mon, May 15, 2017 at 10:20 PM, Tim Armstrong < > [email protected] > > > > > > wrote: > > > > > > > Not sure - if you tell us your username on Apache JIRA we could just > > > assign > > > > it to you. > > > > > > > > On Mon, May 15, 2017 at 6:13 PM, Vincent Tran <[email protected]> > > > wrote: > > > > > > > > > Thanks for the tips. I will go with option 3. > > > > > > > > > > Also, for some reasons, I don't have permissions to assign issues > to > > > > myself > > > > > in the Impala project. Do you guys know if I need to request > > additional > > > > > privileges? > > > > > > > > > > I can assign Hive issues to myself without additional actions. > > > > > > > > > > > > > > > Vincent > > > > > > > > > > > > > > > On Mon, May 15, 2017 at 6:55 PM, Tim Armstrong < > > > [email protected]> > > > > > wrote: > > > > > > > > > > > Also if you're starting to work on the JIRA, can assign it to > > > yourself > > > > in > > > > > > the Apache JIRA instance so we can see that it's assigned to > > someone. > > > > > > > > > > > > Cheers, > > > > > > Tim > > > > > > > > > > > > On Mon, May 15, 2017 at 4:21 PM, Alexander Behm < > > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > Hi Vincent, > > > > > > > > > > > > > > Jim is correct, we to be able to handle invocations with those > > > types: > > > > > > > The 1st arg can be any type. The 2nd and 3rd types must be > > > > compatible. > > > > > > > During function resolution the FE will pick the most > appropriate > > > > > > signature > > > > > > > and add casts to the 2nd or 3rd arg as necessary. > > > > > > > You do not need to create signatures like "nvl2(string, int, > > > > > timestamp)". > > > > > > > > > > > > > > I have considered the following options for addressing this > > > > difficulty: > > > > > > > 1. Accept an AnyType for the 1st argument and create 9 function > > > > > > signatures. > > > > > > > The FE current does not have such a concept. Adding it would > > > involve > > > > > > > changing many tricky places. > > > > > > > 2. Rewrite NVL2() as an IF in the FE using the ExprRewriteRule > > > > > framework. > > > > > > > You will need to make changes to FunctionCallExpr to accept > > NVL2(). > > > > > > > 3. Stamp out all 9*9 function signatures. Should be easy since > > you > > > > are > > > > > in > > > > > > > Python code there. > > > > > > > > > > > > > > Option 3 seems easiest/preferable to me at this point. > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, May 15, 2017 at 1:05 PM, Jim Apple < > [email protected] > > > > > > > > wrote: > > > > > > > > > > > > > > > It seems to me like there should be version with type X, > > [P,X,X] > > > > for > > > > > > > > all pair P and X. There might be a smart way to do this > without > > > the > > > > > > > > quadratic number of cases; someone else probably knows better > > > than > > > > I > > > > > > > > do about that. > > > > > > > > > > > > > > > > On Sun, May 14, 2017 at 1:00 AM, Vincent Tran < > > > [email protected] > > > > > > > > > > > > wrote: > > > > > > > > > Hey folks, > > > > > > > > > > > > > > > > > > I want to start contributing to learn the code base. I > added > > > this > > > > > > > > function > > > > > > > > > to my personal build: > > > > > > > > > > > > > > > > > > [['nvl2'], 'TINYINT', ['TINYINT', 'TINYINT', 'TINYINT'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'SMALLINT', ['SMALLINT', 'SMALLINT', > > 'SMALLINT'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'INT', ['INT', 'INT', 'INT'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'BIGINT', ['BIGINT', 'BIGINT', 'BIGINT'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'FLOAT', ['FLOAT', 'FLOAT', 'FLOAT'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'DOUBLE', ['DOUBLE', 'DOUBLE', 'DOUBLE'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'DECIMAL', ['DECIMAL', 'DECIMAL', 'DECIMAL'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'STRING', ['STRING', 'STRING', 'STRING'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > [['nvl2'], 'TIMESTAMP', ['TIMESTAMP', 'TIMESTAMP', > > > > 'TIMESTAMP'], > > > > > > > > > 'impala::ConditionalFunctions::NVL2'], > > > > > > > > > > > > > > > > > > > > > > > > > > > Do you think this is a sound approach? Should we allow mix > > > types > > > > > for > > > > > > > this > > > > > > > > > function? > > > > > > > > > > > > > > > > > > i.e. nvl2(string, int, timestamp) > > > > > > > > > > > > > > > > > > > > > > > > > > > https://issues.cloudera.org/browse/IMPALA-5030 > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Vincent T. Tran > > > > > > > > > Customer Operations Engineer > > > > > > > > > Cloudera, Inc. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Vincent T. Tran > > > > > Customer Operations Engineer > > > > > Cloudera, Inc. > > > > > > > > > > > > > > > > > > > > > -- > > > Vincent T. Tran > > > Customer Operations Engineer > > > Cloudera, Inc. > > > > > > > > > -- > Vincent T. Tran > Customer Operations Engineer > Cloudera, Inc. >
