trafodion-codereview mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From traflm <...@git.apache.org>
Subject [GitHub] incubator-trafodion pull request #642: [TRAFODION-2119] create table report ...
Date Sat, 06 Aug 2016 01:56:11 GMT
Github user traflm commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/642#discussion_r73778733
  
    --- Diff: core/sql/common/CharType.cpp ---
    @@ -1217,7 +1217,7 @@ void CharType::minMaxRepresentableValue(void* bufPtr,
       if (stringLiteral)
         {
           NABoolean isNull = FALSE;
    -      NABoolean res = createSQLLiteral((const char *) bufPtr, *stringLiteral, isNull,
h);
    +      NABoolean res = createSQLLiteral((const char *) bufPtr  - getSQLnullHdrSize(),
*stringLiteral, isNull, h);
    --- End diff --
    
    The calling to type->minRepresentableValue should pass the buf address that take null
indicator buffer into account, good examples are in:
    -EncodedValue::minMaxValue()
    -ConstValue::ConstValue()
    
    As you can see in above two functions, the buffer ptr is set to the postion:
    buffer_start + null_header_len
    
    a buffer should like
    [nullHeader][VCHARLEN]Data  
    
    inside minRepresentableValue , it will treat the VCHARLEN, so it seems the agreement is
the nullHeader is already bypassed.
    
    So the starting position of bufPtr is
    [nullHeader][VCHARLEN]Data  
                       ^
    
    However, in the createSQLLiteral() , it will again bypass the nullHeader:
      const char *valPtr = buf + getSQLnullHdrSize();
    
    So I did this change. As you correctly pointed out, this is dangerous... 
    A better way is to change createSQLLiteral, but there are some other places call   createSQLLiteral()
which has different assumption of the starting position of the buffer ptr, ConstValue::getConstStr()
for example, so I choose the simplest way, I am more nervous to change many other code which
I don't understand where will be invoked. But yes, it is not good. 
    A better change is to fix in createSQLLiteral semantics, the input buffer ptr will be
the position after nullHeader, but that will involve more changes which I feel not confident,
but I will try.


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