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 2018/08/07 01:48:51 UTC
[GitHub] trafodion pull request #1682: [TRAFODION-3177] Error when selecting count(*)...
GitHub user zellerh opened a pull request:
https://github.com/apache/trafodion/pull/1682
[TRAFODION-3177] Error when selecting count(*) from event_log_reader UDF
See the JIRA for a description of the issues.
- Removed code that required usage of a set of output columns
to evaluate code. This check caused the error described in the
test case, and I think it is no longer necessary. Removing it
can speed up some cases where we now evaluate predicates in the
UDF, for example:
```
select count(*)
from udf("_LIBMGR_".event_log_reader('f'))
where log_file_node = 0;
```
- Added predicate evaluation code for MESSAGE, LOG_FILE_NODE,
LOG_FILE_LINE, and PARSE_STATUS columns.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zellerh/trafodion bug/R23a
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1682.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 #1682
----
commit 5ecb6890cb9fc381bca7c4f5c93ea8a28ed9fd31
Author: Hans Zeller <hz...@...>
Date: 2018-08-07T01:20:09Z
[TRAFODION-3177] Error when selecting count(*) from event_log_reader UDF
See the JIRA for a description of the issues.
- Removed code that required usage of a set of output columns
to evaluate code. This check caused the error described in the
test case, and I think it is no longer necessary. Removing it
can speed up some cases where we now evaluate predicates in the
UDF, for example:
select count(*)
from udf("_LIBMGR_".event_log_reader('f'))
where log_file_node = 0;
- Added predicate evaluation code for MESSAGE, LOG_FILE_NODE,
LOG_FILE_LINE, and PARSE_STATUS columns.
----
---
[GitHub] trafodion pull request #1682: [TRAFODION-3177] Error when selecting count(*)...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1682#discussion_r213030480
--- Diff: core/sql/sqludr/SqlUdrPredefLogReader.cpp ---
@@ -1311,17 +1362,19 @@ bool ReadCppEventsUDFInterface::validateEvent(const UDRInvocationInfo &info,
// All other comparisons are assumed to be string compares
else
{
- // convert predicate value
+ // convert and trim predicate value
temp = constStr;
constStr.clear();
for(size_t j = 0; j < temp.size(); ++j)
constStr += (std::toupper(temp[j]));
+ constStr.erase(constStr.find_last_not_of(" ")+1);
--- End diff --
That's a very good point. Actually, when I made the change I googled "trim string C++" and found something like this: https://stackoverflow.com/questions/216823/whats-the-best-way-to-trim-stdstring.
Then I just copied the solution without thinking too much of it.
When I debug it, it works fine and does not delete the entire string, but I'm not sure why. Adding 1 to string::npos clearly doesn't seem like a good idea.
I'll change the code.
---
[GitHub] trafodion pull request #1682: [TRAFODION-3177] Error when selecting count(*)...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1682#discussion_r210397770
--- Diff: core/sql/sqludr/SqlUdrPredefLogReader.cpp ---
@@ -1311,17 +1362,19 @@ bool ReadCppEventsUDFInterface::validateEvent(const UDRInvocationInfo &info,
// All other comparisons are assumed to be string compares
else
{
- // convert predicate value
+ // convert and trim predicate value
temp = constStr;
constStr.clear();
for(size_t j = 0; j < temp.size(); ++j)
constStr += (std::toupper(temp[j]));
+ constStr.erase(constStr.find_last_not_of(" ")+1);
--- End diff --
What happens when find_last_not_of returns string::npos? (that is, when there are no trailing blanks?) It seems string::npos is defined as -1 (unsigned), so adding 1 to it would get zero and we'd do erase(0) which erases the whole string?
---
[GitHub] trafodion pull request #1682: [TRAFODION-3177] Error when selecting count(*)...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1682
---
[GitHub] trafodion pull request #1682: [TRAFODION-3177] Error when selecting count(*)...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1682#discussion_r213034998
--- Diff: core/sql/sqludr/SqlUdrPredefLogReader.cpp ---
@@ -1311,17 +1362,19 @@ bool ReadCppEventsUDFInterface::validateEvent(const UDRInvocationInfo &info,
// All other comparisons are assumed to be string compares
else
{
- // convert predicate value
+ // convert and trim predicate value
temp = constStr;
constStr.clear();
for(size_t j = 0; j < temp.size(); ++j)
constStr += (std::toupper(temp[j]));
+ constStr.erase(constStr.find_last_not_of(" ")+1);
--- End diff --
Actually, changing the code and debugging again made me realize what happens: For a string with something other than blanks, this will always return something other than string::npos. If the string contains all blanks or is empty, it will return -1 (string::npos), and adding 1 will give 0 and erase the entire string, which is again what we want. But it's ugly to rely on string::npos + 1 = 0, so I'll change it anyway. The resulting code looks ugly as well. It's disappointing that std::string makes it so hard to do a simple operation like trim.
---