db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "A B (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-1623) Add ANSI TRIM implementation
Date Tue, 05 Jun 2007 18:31:26 GMT

    [ 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.


Mime
View raw message