Skip to content
This repository has been archived by the owner on Jun 7, 2021. It is now read-only.

[TRAFODION-3099] Prepare execute fails to handle N'<string>', _iso88591'<string>', _utf8'<string>' for a nchar column #1623

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[TRAFODION-3099] Prepare execute fails to handle N'<string>', _iso88591'<string>', _utf8'<string>' for a nchar column #1623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 95 additions & 2 deletions core/sql/cli/CliExpExchange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,7 +1603,7 @@ InputOutputExpr::outputValues(atp_struct *atp,
sourceType = REC_DECIMAL_LSE;
}

// 5/18/98: added to handle SJIS encoding charset case.
// 5/18/98: added to handle SJIS encoding charset case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor comment, but if you plan to make more changes in Trafodion, you should set your editor to display 8 spaces for each tab, so the indentation is correct. It's good that your editor uses spaces and not tabs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok， I will set my editor each tab to 8 spaces.

// This is used in ODBC where the target character set is still
// ASCII but the actual character set used is SJIS.
//
Expand Down Expand Up @@ -3383,6 +3383,94 @@ InputOutputExpr::inputRowwiseRowsetValues(atp_struct *atp,
return ex_expr::EXPR_ERROR;
}

void handleCharsetPerfix(Descriptor * inputDesc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "Perfix" it should say "Prefix". Again, a minor comment. There are several other places below where the same comment applies.

{
char perfix[MAX_CHAR_SET_STRING_LENGTH];

if(inputDesc)
{
for (Lng32 entry = 1; entry <= inputDesc->getUsedEntryCount(); ++entry)
{
Lng32 perfix_beg = -1;
Lng32 perfix_end = 0;
char* source = inputDesc->getVarData(entry);
Lng32 valueLength = inputDesc->getVarDataLength(entry);
if (source)
{
// get charset perfix beg loc
for (Lng32 i = 0; i < valueLength && i < MAX_CHAR_SET_STRING_LENGTH; ++i)
{
if(source[i] == '_' || TOUPPER(source[i]) =='N')
{
perfix_beg = i;
break;
}

if (source[i] == '\'')
return;
}

if (perfix_beg < 0)
return;

// get charset perfix end loc
Lng32 perfix_ind = 0;

if (source[perfix_beg] == 'N')
{
perfix[perfix_ind] = 'N';
perfix_ind = 1;
}

for (Lng32 i = perfix_beg+1; i < valueLength && i < MAX_CHAR_SET_STRING_LENGTH; ++i)
{
if(source[i] != '\'' && source[i] != 0)
{
perfix[perfix_ind] = TOUPPER(source[i]);
++perfix_ind;
}

if(source[i] == '\'')
{
perfix[perfix_ind] = 0;
perfix_end = i;
break;
}
}

//perfix_cs
CharInfo::CharSet cs = CharInfo::getCharSetEnum(perfix);
if(str_len(perfix) == 1 AND perfix[0] == 'N')
cs = CharInfo::UNICODE;

//if perfix_cs is UnknownCharSet direct return
if(cs == CharInfo::UnknownCharSet)
return;

// remove cs
Lng32 valEnd = 0;
for (Lng32 i = perfix_end+1; i < valueLength; ++i)
{
if (source[i] == '\'')
{
valEnd = i-1;
break;
}
}
Lng32 valBeg = perfix_end+1;
if (!source[valBeg])
{
valBeg += 1;
}
memcpy(&source[perfix_beg], &source[valBeg], valEnd-valBeg+1);
memcpy(&source[valEnd-valBeg+1+perfix_beg], &source[valEnd+3], valBeg+2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this copy uninitialized memory beyond the end of the value? This also shows a problem, that you would need to shorten the string. Another indicator that maybe the bug is in trafci and not here (see comment below).

}

}

}
}

ex_expr::exp_return_type
InputOutputExpr::inputValues(atp_struct *atp,
void * inputDesc_,
Expand Down Expand Up @@ -3639,6 +3727,7 @@ InputOutputExpr::inputValues(atp_struct *atp,

// do the conversion
source = (char *)&targetRowsetSize;

if (::convDoIt(source,
sizeof(Lng32),
REC_BIN32_SIGNED,
Expand Down Expand Up @@ -3804,7 +3893,10 @@ InputOutputExpr::inputValues(atp_struct *atp,
if (!source) {
continue;
}

if(isOdbc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it is correct that JDBC passses a string of the format _iso88591'ABCD' here. I would have expected that trafci should have already converted this to an unquoted string. You can try the following with your fix and see what you get:

trafci
execute xx using '_iso88591''ABCD''';

Will this insert ABCD into the table or _iso88591'ABCD'? If it inserts ABCD, then I think the real bug is in trafci, not in this method, InputOutputExpr::inputValues().

Copy link
Author

@gdgy gdgy Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have test if we not remove the prefix the prefix will insert into the table.
tim 20180628093557

Copy link
Contributor

@zellerh zellerh Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite was I asked: My statement had quotes around the string. Would you be able to try this command with the quoted string, on a system that has your fix:

execute xx using '_iso88591''ABCD''';

trafci does not require quotes around string literals in the "using" clause. So, when it sees _iso88591'ABCD' it interprets that as a string to be inserted. It does not use an SQL parser to parse the string. That is not a bug, in my opinion. It is the correct behavior.

I don't think we should allow SQL syntax other than simple constant values in the "using" clause in trafci.

In other words, I think that TRAFODION-3099 is not an executor bug. You could change trafci to recognize this specific case, but I don't see a really good reason why.

{
handleCharsetPerfix(inputDesc);
}
if (DFS2REC::isSQLVarChar(sourceType)) {
Lng32 vcIndLen = inputDesc->getVarIndicatorLength(entry);
sourceLen = ExpTupleDesc::getVarLength(source, vcIndLen);
Expand Down Expand Up @@ -4408,6 +4500,7 @@ ex_expr::exp_return_type InputOutputExpr::addDescInfoIntoStaticDesc
}

return ex_expr::EXPR_OK;

}

Lng32 InputOutputExpr::getMaxParamIdx()
Expand Down
5 changes: 4 additions & 1 deletion core/sql/sqlci/SqlCmd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2673,7 +2673,7 @@ Lng32 Execute::storeParams(char* argument_, short &num_params,
char param[MAX_LEN_UNNAMED_PARAM+1]; // buffer for current value
size_t i = 0; // len(param)+1

if ( using_param_charsets && *args == '_' ) {
if ( using_param_charsets && (*args == '_' || TOUPPER(*args) == 'N')) {
char* prefixPtr = args+1;
while ( *prefixPtr ) {
if ( *prefixPtr == '\'' ) {
Expand All @@ -2692,6 +2692,9 @@ Lng32 Execute::storeParams(char* argument_, short &num_params,

// name lookup
CharInfo::CharSet cs = CharInfo::getCharSetEnum(upperCaseName);
if ( TOUPPER(*args) == 'N') {
cs = CharInfo::UNICODE;
}
delete [] upperCaseName;

if (CharInfo::isCharSetFullySupported(cs)) {
Expand Down