You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by zellerh <gi...@git.apache.org> on 2015/08/17 23:55:41 UTC

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

GitHub user zellerh opened a pull request:

    https://github.com/apache/incubator-trafodion/pull/60

    TRAFODION-1457 Issues related to decoding HBase region keys and TEXT …

    Fixed a few issues relating to decoding binary encoded key values of UTF8 and interval data type.
    Also moved the logic of double-quoting strings into the methods that update the TEXT table.
    Renamed some regression files that used obsolete suffixes like .LINUX.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/zellerh/incubator-trafodion bug/1129

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-trafodion/pull/60.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #60
    
----
commit 1e60a9b8cd487698c44659663ad09ddc455c3708
Author: Hans Zeller <ha...@esgyn.com>
Date:   2015-08-17T21:43:31Z

    TRAFODION-1457 Issues related to decoding HBase region keys and TEXT table

----


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37244951
  
    --- Diff: core/sql/optimizer/BindItemExpr.cpp ---
    @@ -10298,23 +10298,23 @@ NABoolean ZZZBinderFunction::isPadWithSpace (ExprValueId& padExpr, CharInfo::Cha
       if ((padExpr->castToItemExpr()->getOperatorType() == ITM_CONSTANT) &&
           (!((ConstValue *)padExpr->castToItemExpr())->getText().isNull()))
       {
    -     NAWString padString(
    -       ((ConstValue *)padExpr->castToItemExpr())->getConstWStr());
    +     NAString padString(
    +       ((ConstValue *)padExpr->castToItemExpr())->getConstStr(FALSE));
          Int32 i = 0;
          NABoolean foundSingleQuote = FALSE;
    -     for (const NAWchar *s = padString.data(); *s; s++)
    +     for (const char *s = padString.data(); *s; s++)
          {
    -       i++;  // 0x0027 is the single quote character '
    -       if ((!foundSingleQuote)&&(*s != 0x0027)) // loop through 
    +       i++;  // 0x27 is the single quote character '
    +       if ((!foundSingleQuote)&&(*s != 0x27)) // loop through 
              continue;  // the prefix _UCS2 or _ISO88591
            else if ((!foundSingleQuote))
            {
             foundSingleQuote = TRUE; // found the leading single quote.
             continue;
            }
    -       if ((i == (padString.length())) && (*s == 0x0027)) // trailing single quote
    +       if ((i == (padString.length())) && (*s == 0x27)) // trailing single quote
              continue;
    -       if (*s != 0x0020) // 0x0020 is the single space character ' '
    +       if (*s != 0x20) // 0x20 is the single space character ' '
    --- End diff --
    
    Again, slightly more portable to code *s != ' ' 


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37244917
  
    --- Diff: core/sql/optimizer/BindItemExpr.cpp ---
    @@ -10298,23 +10298,23 @@ NABoolean ZZZBinderFunction::isPadWithSpace (ExprValueId& padExpr, CharInfo::Cha
       if ((padExpr->castToItemExpr()->getOperatorType() == ITM_CONSTANT) &&
           (!((ConstValue *)padExpr->castToItemExpr())->getText().isNull()))
       {
    -     NAWString padString(
    -       ((ConstValue *)padExpr->castToItemExpr())->getConstWStr());
    +     NAString padString(
    +       ((ConstValue *)padExpr->castToItemExpr())->getConstStr(FALSE));
          Int32 i = 0;
          NABoolean foundSingleQuote = FALSE;
    -     for (const NAWchar *s = padString.data(); *s; s++)
    +     for (const char *s = padString.data(); *s; s++)
          {
    -       i++;  // 0x0027 is the single quote character '
    -       if ((!foundSingleQuote)&&(*s != 0x0027)) // loop through 
    +       i++;  // 0x27 is the single quote character '
    +       if ((!foundSingleQuote)&&(*s != 0x27)) // loop through 
    --- End diff --
    
    Slightly more portable to code *s != '\''  (it would work in EBCDIC, for example, while 0x27 would not).


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37246073
  
    --- Diff: core/sql/optimizer/NATable.cpp ---
    @@ -1472,58 +1478,161 @@ static ItemExpr * getRangePartitionBoundaryValuesFromEncodedKeys(
                   // value. Note that the buffer has a prefix of some bytes with actual key
                   // values, followed by bytes that are zeroed out. 
     
    +              // the number of bytes actually provided in the key (not filled in)
    +              int numBytesInProvidedVal = encodedKeyLen-valOffset;
    +
    +              if (nullHdrSize && numBytesInProvidedVal <= 0)
    --- End diff --
    
    The condition numBytesInProvidedVal <= 0 should always be true by definition of columnIsPartiallyProvided. Doesn't hurt anything though, just makes reading more complicated.


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-trafodion/pull/60


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37246384
  
    --- Diff: core/sql/optimizer/BindItemExpr.cpp ---
    @@ -10298,23 +10298,23 @@ NABoolean ZZZBinderFunction::isPadWithSpace (ExprValueId& padExpr, CharInfo::Cha
       if ((padExpr->castToItemExpr()->getOperatorType() == ITM_CONSTANT) &&
           (!((ConstValue *)padExpr->castToItemExpr())->getText().isNull()))
       {
    -     NAWString padString(
    -       ((ConstValue *)padExpr->castToItemExpr())->getConstWStr());
    +     NAString padString(
    +       ((ConstValue *)padExpr->castToItemExpr())->getConstStr(FALSE));
          Int32 i = 0;
          NABoolean foundSingleQuote = FALSE;
    -     for (const NAWchar *s = padString.data(); *s; s++)
    +     for (const char *s = padString.data(); *s; s++)
          {
    -       i++;  // 0x0027 is the single quote character '
    -       if ((!foundSingleQuote)&&(*s != 0x0027)) // loop through 
    +       i++;  // 0x27 is the single quote character '
    +       if ((!foundSingleQuote)&&(*s != 0x27)) // loop through 
    --- End diff --
    
    Done.


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37246902
  
    --- Diff: core/sql/optimizer/NATable.cpp ---
    @@ -1472,58 +1478,161 @@ static ItemExpr * getRangePartitionBoundaryValuesFromEncodedKeys(
                   // value. Note that the buffer has a prefix of some bytes with actual key
                   // values, followed by bytes that are zeroed out. 
     
    +              // the number of bytes actually provided in the key (not filled in)
    +              int numBytesInProvidedVal = encodedKeyLen-valOffset;
    +
    +              if (nullHdrSize && numBytesInProvidedVal <= 0)
    --- End diff --
    
    Couldn't this happen, for example?
    
    currOffset = 10
    encodedKeyLen = 11
    nullHdrSize = 2
    valOffset = 12
    
    In other words, the partially provided key covers only the NULL indicator or part of it.


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37246397
  
    --- Diff: core/sql/optimizer/BindItemExpr.cpp ---
    @@ -10298,23 +10298,23 @@ NABoolean ZZZBinderFunction::isPadWithSpace (ExprValueId& padExpr, CharInfo::Cha
       if ((padExpr->castToItemExpr()->getOperatorType() == ITM_CONSTANT) &&
           (!((ConstValue *)padExpr->castToItemExpr())->getText().isNull()))
       {
    -     NAWString padString(
    -       ((ConstValue *)padExpr->castToItemExpr())->getConstWStr());
    +     NAString padString(
    +       ((ConstValue *)padExpr->castToItemExpr())->getConstStr(FALSE));
          Int32 i = 0;
          NABoolean foundSingleQuote = FALSE;
    -     for (const NAWchar *s = padString.data(); *s; s++)
    +     for (const char *s = padString.data(); *s; s++)
          {
    -       i++;  // 0x0027 is the single quote character '
    -       if ((!foundSingleQuote)&&(*s != 0x0027)) // loop through 
    +       i++;  // 0x27 is the single quote character '
    +       if ((!foundSingleQuote)&&(*s != 0x27)) // loop through 
              continue;  // the prefix _UCS2 or _ISO88591
            else if ((!foundSingleQuote))
            {
             foundSingleQuote = TRUE; // found the leading single quote.
             continue;
            }
    -       if ((i == (padString.length())) && (*s == 0x0027)) // trailing single quote
    +       if ((i == (padString.length())) && (*s == 0x27)) // trailing single quote
              continue;
    -       if (*s != 0x0020) // 0x0020 is the single space character ' '
    +       if (*s != 0x20) // 0x20 is the single space character ' '
    --- End diff --
    
    Done.


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

[GitHub] incubator-trafodion pull request: TRAFODION-1457 Issues related to...

Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:

    https://github.com/apache/incubator-trafodion/pull/60#discussion_r37247350
  
    --- Diff: core/sql/sqlcomp/CmpSeabaseDDLcommon.cpp ---
    @@ -6137,53 +6138,82 @@ short CmpSeabaseDDL::validateDivisionByExprForDDL(ItemExpr *divExpr)
     }
     
     short CmpSeabaseDDL::createEncodedKeysBuffer(char** &encodedKeysBuffer,
    +                                             int &numSplits,
                                                  desc_struct * colDescs, 
                                                  desc_struct * keyDescs,
    -                                             Lng32 numSplits, Lng32 numKeys, 
    -                                             Lng32 keyLength, NABoolean isIndex)
    +                                             int numSaltPartitions,
    +                                             Lng32 numSaltSplits,
    +                                             NAString *splitByClause,
    +                                             Lng32 numKeys, 
    +                                             Lng32 keyLength,
    +                                             NABoolean isIndex)
     {
       encodedKeysBuffer = NULL;
    -  
    -  if (numSplits <= 0)
    +  numSplits = 0;
    +
    +  if (numSaltSplits <= 0)
         return 0;
     
    -    NAString ** inArray = createInArrayForLowOrHighKeys(colDescs, 
    -                                                        keyDescs,
    -                                                        numKeys,
    -                                                        FALSE,
    -                                                        isIndex,
    -                                                        STMTHEAP ); 
    -
    -    char splitNumCharStr[5];
    -    NAString splitNumString;
    -
    -    /* HBase creates 1 more split than
    -       the number of rows in the split array. In the example below we have a 
    -       salt column and an integer column as the key. KeyLength is 4 + 4 = 8.
    -       encodedKeysBuffer will have 4 elements, each of length 8. When this
    -       buffer is given to HBase through the Java API we get a table with 5 
    -       splits and begin/end keys as shown below
    -       
    -       Start Key                                                     End Key
    -       \x00\x00\x00\x01\x00\x00\x00\x00
    -       \x00\x00\x00\x01\x00\x00\x00\x00      \x00\x00\x00\x02\x00\x00\x00\x00   
    -       \x00\x00\x00\x02\x00\x00\x00\x00      \x00\x00\x00\x03\x00\x00\x00\x00
    -       \x00\x00\x00\x03\x00\x00\x00\x00      \x00\x00\x00\x04\x00\x00\x00\x00   
    -       \x00\x00\x00\x04\x00\x00\x00\x00 
    -    */
    -    encodedKeysBuffer = new (STMTHEAP) char*[numSplits];
    -    for(int i =0; i < numSplits; i++)
    -      encodedKeysBuffer[i] = new (STMTHEAP) char[keyLength];
    -
    -    inArray[0] = &splitNumString;
    -    short retVal = 0;
    -    
    -    for(Int32 i =0; i < numSplits; i++) {
    -      sprintf(splitNumCharStr, "%d", i+1);
    -      splitNumString = splitNumCharStr;
    +  // make a list of NAStrings with the default values for start keys
    +  NAString ** defaultSplits = createInArrayForLowOrHighKeys(
    +       colDescs,
    +       keyDescs,
    +       numKeys,
    +       FALSE,
    +       isIndex,
    +       STMTHEAP );
    +  NAString **splitValuesAsText = new(STMTHEAP) NAString *[numKeys];
    +  ElemDDLPartitionList * pPartitionList = NULL;
    +  ElemDDLPartitionRange *pPartitionRange = NULL;
    +
    +      numSplits = numSaltSplits;
    +
    +      // for salt splits, only the first key column value
    +      // is variable, the rest use the default values
    +      for (int k=1; k<numKeys; k++)
    +        splitValuesAsText[k] = defaultSplits[k];
    +
    --- End diff --
    
    Indentation is inconsistent (if you happen to fix something else perhaps you could fix this too)


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