You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by DaveBirdsall <gi...@git.apache.org> on 2017/01/17 17:15:25 UTC
[GitHub] incubator-trafodion pull request #915: [TRAFODION-2440] Replace fixed length...
GitHub user DaveBirdsall opened a pull request:
https://github.com/apache/incubator-trafodion/pull/915
[TRAFODION-2440] Replace fixed length buffers with strings in LOB code
The LOB code has two uses:
1. To read and write LOB column values in Trafodion tables
2. To read Hive files into the Trafodion executor
The LOB code made use of fixed length buffers to store LOB file names. Unfortunately these are too short in general for use with Hive files. Rather than simply make them longer, I have replaced these fixed length buffers with string data types in most places in the code.
Some design notes:
The LOB code is in the middle of a slow transition from using STL classes and the global heap to using the Trafodion NAHeap classes and heaps for memory management. Where a LOB class uses the STL heap, I replaced its fixed length buffer members with an STL string. Where a LOB class uses NAHeap, I used a Trafodion NAString instead.
In one place I left the fixed length buffer alone: I did this where the LOB code calls the CLI function SQL_EXEC_LOBcliInterface. It appears that this code path is traversed only for LOB column values. (If anyone knows otherwise, please comment.)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaveBirdsall/incubator-trafodion Trafodion2440
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/915.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 #915
----
commit 49498dfdf6b629bf349f9630fdb5ceb197e93bf9
Author: Dave Birdsall <db...@apache.org>
Date: 2017-01-17T17:07:11Z
[TRAFODION-2440] Replace fixed length buffers with strings in LOB code
----
---
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 #915: [TRAFODION-2440] Replace fixed length...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/915#discussion_r96522147
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -198,7 +198,7 @@ Ex_Lob_Error ExLob::fetchCursor(char *handleIn, Int32 handleLenIn, Int64 &outOff
lobCursors_it it = lobCursors_.find(string(handleIn, handleLenIn));
char logBuf[4096];
lobDebugInfo("In ExLob::fetchCursor",0,__LINE__,lobTrace_);
- char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6];
+ char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6]; // TODO: do we ever go this way for Hive?
Int32 blackBoxLen = 0;
--- End diff --
This code path is taken for external LOB files. The file name could be provided by the user in the form of file:///<path name>/.../<filename> So perhaps best to change 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.
---
[GitHub] incubator-trafodion pull request #915: [TRAFODION-2440] Replace fixed length...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/915#discussion_r96711995
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -515,7 +515,7 @@ class ExLob
public:
- char lobDataFile_[MAX_LOB_FILE_NAME_LEN];
+ string lobDataFile_; // TODO: change to NAString when ExLobCursor is allocated off of an NAHeap
--- End diff --
Can this not be allocated from NAString now ? The ExLob class which contains this is allocated from the executor heap.
---
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 #915: [TRAFODION-2440] Replace fixed length...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/915#discussion_r97351150
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -198,7 +199,7 @@ Ex_Lob_Error ExLob::fetchCursor(char *handleIn, Int32 handleLenIn, Int64 &outOff
lobCursors_it it = lobCursors_.find(string(handleIn, handleLenIn));
char logBuf[4096];
lobDebugInfo("In ExLob::fetchCursor",0,__LINE__,lobTrace_);
- char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6];
+ char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6]; // TODO: do we ever go this way for Hive?
--- End diff --
I think this comment can be removed now. Hive does not go through this.
---
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 #915: [TRAFODION-2440] Replace fixed length...
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/915#discussion_r97354930
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -2285,7 +2338,7 @@ Ex_Lob_Error ExLobsOper (
{
Ex_Lob_Error err = LOB_OPER_OK;
ExLob *lobPtr = NULL;
- char fn[MAX_LOB_FILE_NAME_LEN];
+ string fn;
--- End diff --
Missed this one. This needs to be converted to a dynamic char array or an NAString on the Lob globals heap.
---
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 #915: [TRAFODION-2440] Replace fixed length...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/915
---
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 #915: [TRAFODION-2440] Replace fixed length...
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/915#discussion_r96537364
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -198,7 +198,7 @@ Ex_Lob_Error ExLob::fetchCursor(char *handleIn, Int32 handleLenIn, Int64 &outOff
lobCursors_it it = lobCursors_.find(string(handleIn, handleLenIn));
char logBuf[4096];
lobDebugInfo("In ExLob::fetchCursor",0,__LINE__,lobTrace_);
- char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6];
+ char *blackBox = new(getLobGlobalHeap()) char[MAX_LOB_FILE_NAME_LEN+6]; // TODO: do we ever go this way for Hive?
Int32 blackBoxLen = 0;
--- End diff --
OK. Will change.
---
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 #915: [TRAFODION-2440] Replace fixed length...
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/915#discussion_r96712914
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -515,7 +515,7 @@ class ExLob
public:
- char lobDataFile_[MAX_LOB_FILE_NAME_LEN];
+ string lobDataFile_; // TODO: change to NAString when ExLobCursor is allocated off of an NAHeap
--- End diff --
Actually, it's not. See line 2338 (new file numbering):
//lobPtr = new (lobGlobals->getHeap())ExLob();
lobPtr = new ExLob();
It appears that someone experimented with putting it on the executor heap and then retreated.
---
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.
---