[
https://issues.apache.org/jira/browse/DERBY-1620?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12501730
]
A B commented on DERBY-1620:
----------------------------
Hi John,
Thank you for following through with my previous review comments, and for
rewriting your test cases to run in JUnit. That was very helpful.
Just a few minor comments on the latest patch:
1 - The Java class name at the top of the license header in the new JUnit test
says "NullIfTest", when it should say "CaseExpressionTest".
2 - There are a few unnecessary imports in CaseExpressionTest:
java.sql.Date
java.sql.PreparedStatement
java.sql.Time
java.sql.Timestamp
org.apache.derbyTesting.junit.JDBC
3 - In the "suite()" method I think it might be good to use existing
convenience methods on TestConfiguration, instead of calling "baseSuite"
directly. Ex:
// For embedded:
suite.addTest(
TestConfiguration.embeddedSuite(CaseExpressionTest.class));
// For client/server:
suite.addTest(
TestConfiguration.clientServerSuite(CaseExpressionTest.class));
// For both embedded *and* client/server:
suite.addTest(
TestConfiguration.defaultSuite(CaseExpressionTest.class));
That said, since the changes for Jira only effect SQL processing within the
engine, it's probably good enough to just run the new JUnit test in embedded
mode.
4 - There are several System.out.printlns in the test. I think that the
preferred approach to JUnit testing is to avoid printing anything to System.out
or System.err. If the output is an important part of the test then is should
be replaced with some kind of JUnit assertion. But in the new
CaseExpressionTest, it looks like the System.out.println statements are purely
informational, in which case I think it's best to remove them altogether. Or,
if you think the output might be useful for debugging, you could move all of
the System.outs into a "debug" method and add a flag that allows debugging
information to be turned on/off (with default to "off"). See, for example,
lang/MathTrigFunctionsTest.java.
5 - I think the new test has to be added to lang/_Suite.java in order to run as
part of Derby's JUnit regression suite. This should just be a one-line
addition to the "suite()" method of lang/_Suite.java, something like:
suite.addTest(CaseExpressionTest.suite());
6 - It might be nice if you can name your next patch something other than
"ConditionalNode.diff", in order to avoid confusion with the changes that have
already been committed. For example, something like "derby1620_test.patch"
would, I think, be a tad more clear.
Thanks again for your continued work (and patience) with this Jira! I think
that if the above comments can be addressed, the patch will be ready for commit
and we can finally close this issue...
> SQL CASE statement returns ERROR 42X89 when including NULL as a return value
> ----------------------------------------------------------------------------
>
> Key: DERBY-1620
> URL: https://issues.apache.org/jira/browse/DERBY-1620
> Project: Derby
> Issue Type: Bug
> Components: SQL
> Affects Versions: 10.2.1.6
> Environment: Windows XP
> Reporter: John Peterson
> Assignee: John Peterson
> Priority: Minor
> Fix For: 10.3.0.0
>
> Attachments: ConditionalNode.diff, ConditionalNode.diff,
> ConditionalNode.diff, ConditionalNode.diff, Derby_Community_Discussion.doc,
> derbyall_report.txt, resultset.tmp, resultset.tmp, sysinfo_and_example.txt
>
>
> This bug appears to be related to the DERBY-7 bug (NULLIF() function). When
> NULL is used during a CASE statement, Derby requires the NULL to be CAST to
> the appropriate type. This does not appear to meet the SQL 2003 Standard for
> the Case Expression (see attached Word document). See the attached Word
> document to view the Derby Community Discussion about this issue. See the
> attached .TXT to view the SYSINFO and to see an example of the steps to
> reproduce using IJ.
> Steps to Reproduce:
> ij>values case when 1=2 then 3 else NULL end;
> ERROR 42X89: Types 'INTEGER' and 'CHAR' are not type compatible. Neither
> type is assignable to the other type.
> Current Workaround:
> ij>values case when 1=2 then 3 else cast(NULL as INT) end;
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
