You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by gdgy <gi...@git.apache.org> on 2018/06/27 01:51:56 UTC
[GitHub] trafodion pull request #1623: TRAFODION-3099
GitHub user gdgy opened a pull request:
https://github.com/apache/trafodion/pull/1623
TRAFODION-3099
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gdgy/trafodion trafodion_3099
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1623.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 #1623
----
commit 1165e78246cfd65047d733d8efb7059106f4eb46
Author: shaoyong.li <li...@...>
Date: 2018-06-25T06:20:55Z
TRAFODION-3099
----
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by gdgy <gi...@git.apache.org>.
Github user gdgy commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198686893
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -3804,7 +3893,10 @@ InputOutputExpr::inputValues(atp_struct *atp,
if (!source) {
continue;
}
-
+ if(isOdbc)
--- End diff --
I have test if we not remove the prefix the prefix will insert into the table.
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198553974
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -3383,6 +3383,94 @@ InputOutputExpr::inputRowwiseRowsetValues(atp_struct *atp,
return ex_expr::EXPR_ERROR;
}
+void handleCharsetPerfix(Descriptor * inputDesc)
+{
+ 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);
--- End diff --
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).
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198544452
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -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.
--- End diff --
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.
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by gdgy <gi...@git.apache.org>.
Github user gdgy commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198683956
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -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.
--- End diff --
ok， I will set my editor each tab to 8 spaces.
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198899742
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -3804,7 +3893,10 @@ InputOutputExpr::inputValues(atp_struct *atp,
if (!source) {
continue;
}
-
+ if(isOdbc)
--- End diff --
Maybe trafci does not require quotes around string literals in the "using" clause. Also, I don't think we should allow this kind of syntax in the "using" clause. We are specifying the value of a parameter that already has a data type. Specifying a character set at this time does not make sense, that syntax is meant for SQL queries, not for variables of parameters.
Note that the "using" clause does not accept SQL syntax, only values. So, execute xx using 3+4 would probably insert '3+4' into the database, not '7'.
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198552129
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -3804,7 +3893,10 @@ InputOutputExpr::inputValues(atp_struct *atp,
if (!source) {
continue;
}
-
+ if(isOdbc)
--- End diff --
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().
---
[GitHub] trafodion pull request #1623: [TRAFODION-3099] Prepare execute fails to hand...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1623#discussion_r198544668
--- Diff: core/sql/cli/CliExpExchange.cpp ---
@@ -3383,6 +3383,94 @@ InputOutputExpr::inputRowwiseRowsetValues(atp_struct *atp,
return ex_expr::EXPR_ERROR;
}
+void handleCharsetPerfix(Descriptor * inputDesc)
--- End diff --
Instead of "Perfix" it should say "Prefix". Again, a minor comment. There are several other places below where the same comment applies.
---