trafodion-codereview mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From DaveBirdsall <...@git.apache.org>
Subject [GitHub] incubator-trafodion pull request #1169: [TRAFODION-2680] Add CSV_FORMAT func...
Date Mon, 17 Jul 2017 16:48:23 GMT
Github user DaveBirdsall commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/1169#discussion_r127760681
  
    --- Diff: core/sql/optimizer/BindItemExpr.cpp ---
    @@ -10851,6 +10851,96 @@ ItemExpr *ZZZBinderFunction::bindNode(BindWA *bindWA)
           }
         break;
     
    +    case ITM_CSV_FORMAT:
    +      {
    +	bindChildren(bindWA);
    +	if (bindWA->errStatus()) 
    +	  return this;
    +
    +        // The way the arguments of CSV_FORMAT are represented in
    +        // the parse tree is as a tree of ItemList nodes; so
    +        // CSV_FORMAT(a,b,c,d) is represented as
    +        //
    +        //    this
    +        //    /  \
    +        //   a   ItemList
    +        //         /  \
    +        //        b   ItemList
    +        //              /  \
    +        //             c    d
    +        //
    +        // The code below traverses accordingly.
    +       
    +        ItemExpr * next = child(0)->castToItemExpr();     
    +        ItemExpr * resultBoundTree = NULL;
    +        while (next)  // while arguments remain to process
    +          {
    +            ItemExpr * childk = NULL;
    +            if (next->getOperatorType() == ITM_ITEM_LIST)
    +              {
    +                childk = next->child(0);
    +                next = next->child(1);
    +              }
    +            else
    +              {
    +                childk = next;
    +                next = NULL;
    +              }
    +           
    +            const NAType &type = childk->getValueId().getType();
    +            switch (type.getTypeQualifier())
    +              {
    +                case NA_BOOLEAN_TYPE:
    +                case NA_DATETIME_TYPE:
    +                case NA_INTERVAL_TYPE:
    +                case NA_NUMERIC_TYPE:
    +                  {
    +                    // TODO: Is VARCHAR(100) big enough? Consider bignums, for example.
We could
    +                    // probably be smarter about the length. E.g. VARCHAR(6) is big enough
for SMALLINT.
    +                    strcpy(buf,"CAST(@A1 AS VARCHAR(100))");
    +                    parseTree = parser.getItemExprTree(buf, strlen(buf), BINDITEMEXPR_STMTCHARSET,
1, childk);
    +                    boundTree = parseTree->bindNode(bindWA);
    +                    if (bindWA->errStatus()) 
    +                      return this;
    +                    break;
    +                  }
    +                case NA_CHARACTER_TYPE:
    +                  {
    +                    boundTree = childk;
    +                    break;
    +                  }
    +                default:
    +                  {
    +                    // operand has an unsupported data type
    +                    *CmpCommon::diags() << DgSqlCode(-4018)
    +                                        << DgString0("CSV_FORMAT")
    +                                        << DgString1(type.getTypeName().data());
    +                    bindWA->setErrStatus();
    +                    return this;
    +                  }
    +              }
    +
    +            strcpy(buf,"CASE WHEN POSITION(',' IN @A1) > 0 THEN '\"' || @A1 || '\"'
ELSE @A1 END");
    --- End diff --
    
    Re: booleans and so on. I think you're right. I need to check if we have a European form
of CAST that uses a comma for the decimal point for numbers. Will look into this.
    
    Re: double quotes: Thanks very much for the tip. I need to do a little experimentation
with Excel spreadsheets to see how they behave with double quotes. Then I'll make changes
accordingly.
    
    Re: white space: I wondered about that. I suspect programs such as Excel do ignore trailing
white space (not sure about leading). Seems like adding TRIM would be redundant in that case?
There is an aesthetic case to be made though in getting rid of the extra white space in the
output.


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