[
https://issues.apache.org/jira/browse/DERBY-1623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12501647
]
A B commented on DERBY-1623:
----------------------------
Hi Manish,
Thank your for your work on this. I did a review of the changes and have the
following comments/questions, in no particular order:
1. Following fails with an NPE. SQL standard says the result should be NULL
(General Rules 9.c as posted by Bernt above).
create table ts (c char(1), vc varchar(10));
insert into ts values (default, default);
-- Note "c" here is null.
select trim(c from ' hmm ') from ts;
-- And similarly...
select trim ((values cast (null as char(1))) from vc) from ts;
2. Error message 22020 says that the trim string cannot be null, but the SQL
standard allows it to be (per General Rules 9.c noted above). Were you
intentionally planning to enforce this restriction, or is this just a typo? If
it was not intentional, then I think the corresponding text can be removed the
the error message.
3. There is an extra space between "string" and "must" in the error message for
22020:
ERROR 22020: Invalid trim string, ''. The trim string must be exactly one
character. It
cannot be a null or more than one character.
4. The SQL 2003 standard indicates that TRIM is a reserved keyword, and one of
your previous comments says that, as well. However, I don't see "TRIM" in the
list of reserved keywords in sqlgrammar.jj. There is a code comment which says:
/* NOTE - If you add a keyword, then you must add it to reservedKeyword()
* or nonReservedKeyword() as well!
*/
and I see that "TRIM" has been added as a keyword with your patch, but I don't
see it listed in either reservedKeyword() nor in nonReservedKeyword(). I also
noticed that this patch adds "TRIM" to the "miscBuiltins()" method, which means
that it can be used (in some form) with the JDBC escape syntax, ex:
values { fn trim(' a ') };
Was that intentional? If so, is this function defined in the JDBC spec like
LTRIM and RTRIM are, and does ANSI behavior satisfy the JDBC requirements?
(sorry for my ignorance here).
6. Is the "trim(...)" method on StringDataValue still used anywhere after these
changes? It looks like you replaced it with "ansiTrim", though I could of
course be overlooking something. If the old "trim" method is no longer used,
would it make sense to remove the code from SQLChar?
7. I'm not entirely sure, but I think that use of statements like the following
in JUnit tests is not ideal:
rs = prepareStatement(sql).executeQuery();
The reason is (I *think*) that the PreparedStatement object is never explicitly
closed. There have been situations in the past where failure to close a
statement leads to problems in the JUnit suite. I can't say what the specifics
are, but if possible I think it would be better to explicitly assign the
prepared statement and close it when appropriate. Fortunately there are only
two such cases in the new test, so this should be pretty easy to change.
8. Do you have any plans to document this new function? It might be good to
create a subtask for tracking, so that the documentation can be completed at
some point (even if it's not complete for 10.3, it would be good to have a Jira
so that we don't lose track of it).
I ran some simple tests by hand and everything seems to work (with the
exception of the NPE noted above). I did not, however, run either of the
nightly suites. Did you run derbyall and suites.All with this patch applied,
and did everything run cleanly?
Thanks again for your time with this feature!
> Add ANSI TRIM implementation
> ----------------------------
>
> Key: DERBY-1623
> URL: https://issues.apache.org/jira/browse/DERBY-1623
> Project: Derby
> Issue Type: Improvement
> Components: SQL
> Reporter: Emmanuel Bernard
> Assignee: Manish Khettry
> Attachments: 1623-parser-guess.diff, 1623.patch.txt,
> AnsiTrimTest.java, compile_error.jj, sqlgrammar.jj.diff
>
>
> JPA is requiring databases to support this ANSI feature esp the ability to
> chose the trimmed character
> TRIM([ [ LEADING | TRAILING | BOTH ] [ chars ] FROM ] str)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
