[TRAFODION-3099] Prepare execute fails to handle N'<string>', _iso88591'<string>', _utf8'<string>' for a nchar column #1623
Conversation
|
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2786/ |
gdgy
changed the title
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2786/ |
| @@ -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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok， I will set my editor each tab to 8 spaces.
| @@ -3383,6 +3383,94 @@ InputOutputExpr::inputRowwiseRowsetValues(atp_struct *atp, | |||
| return ex_expr::EXPR_ERROR; | |||
| } | |||
|
|
|||
| void handleCharsetPerfix(Descriptor * inputDesc) | |||
There was a problem hiding this comment.
Instead of "Perfix" it should say "Prefix". Again, a minor comment. There are several other places below where the same comment applies.
| @@ -3804,7 +3893,10 @@ InputOutputExpr::inputValues(atp_struct *atp, | |||
| if (!source) { | |||
| continue; | |||
| } | |||
|
|
|||
| if(isOdbc) | |||
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
I have test if we not remove the prefix the prefix will insert into the table.
There was a problem hiding this comment.
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.
| valBeg += 1; | ||
| } | ||
| memcpy(&source[perfix_beg], &source[valBeg], valEnd-valBeg+1); | ||
| memcpy(&source[valEnd-valBeg+1+perfix_beg], &source[valEnd+3], valBeg+2); |
There was a problem hiding this comment.
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).
|
I think there is a conceptual issue with this fix and with the original JIRA. The datatype of the dynamic parameter is fixed by the compiler at compile time, so it cannot depend on an expression supplied at run time. There are mechanisms at the CLI level that allow run time manipulation; it may be that ODBC/JDBC allow access to those. (Trafci does not.) But those are limited to changing the datatype only; they do not involve lexical analysis or parsing of the expression. Indeed, given a string 'ABCD' in an EXECUTE USING statement, trafci automatically converts from the character set of the SQL statement to the character set of the column, making the N'xxxx' and _iso88591'xxxx' syntaxes unnecessary. |
|
Following up on that remark: Perhaps the best way to resolve this particular JIRA is with a documentation change. We can simply document this behavior in the SQL Reference Manual. |
|
Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/2878/ |
|
Can one of the admins verify this patch? |
No description provided.