Derby
  1. Derby
  2. DERBY-1620

SQL CASE statement returns ERROR 42X89 when including NULL as a return value

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.3.1.4
    • Component/s: SQL
    • Labels:
      None
    • Environment:
      Windows XP

      Description

      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;

      1. Derby_Community_Discussion.doc
        38 kB
        John Peterson
      2. sysinfo_and_example.txt
        4 kB
        John Peterson
      3. derbyall_report.txt
        27 kB
        John Peterson
      4. ConditionalNode.diff
        13 kB
        John Peterson
      5. ConditionalNode.diff
        8 kB
        John Peterson
      6. resultset.tmp
        98 kB
        John Peterson
      7. ConditionalNode.diff
        13 kB
        John Peterson
      8. resultset.tmp
        102 kB
        John Peterson
      9. ConditionalNode.diff
        14 kB
        John Peterson
      10. derby1620_test.patch
        14 kB
        John Peterson
      11. derby1620_test_v2.patch
        13 kB
        A B

        Issue Links

          Activity

          Hide
          John Peterson added a comment -

          These attachments are the SQL 2003 Standard for the Case Expression, the Derby Community Discussion about this issue, and the SysInfo along with an example of the Steps to Reproduce.

          Show
          John Peterson added a comment - These attachments are the SQL 2003 Standard for the Case Expression, the Derby Community Discussion about this issue, and the SysInfo along with an example of the Steps to Reproduce.
          Show
          Jean T. Anderson added a comment - For any who don't have Word, the start for the discussion in Derby_Community_Discussion.doc can be viewed at these locations: ASF archives: http://mail-archives.apache.org/mod_mbox/db-derby-user/200607.mbox/%3cD867A529C569F64BAC09C622E55B4084010E2BF8@EXCHCLUSTUSA.rpega.com%3e http://tinyurl.com/lceff Nabble: http://www.nabble.com/ERROR-42X89%3A-Types-%27INTEGER%27-and-%27CHAR%27-are-not-type-compatible.-Neither-type-is-assignable-to-the-other-type.-p5527265.html http://tinyurl.com/oomep
          Hide
          John Peterson added a comment -

          It has been brought to my attention that the SQL2003_Standard_Case_Expression.doc is a violation of the ISO copyright, I suggest it be removed as soon as possible. Instead, please refer to http://en.wikipedia.org/wiki/SQL:2003 for a link to a .zip file (sql_2003_standard.zip), and open the 5WD-02-Foundation-2003-09.pdf within. Turn to page 197 (221 of 1332) (Chapter 6.11) to view the Case Expression standard.

          Show
          John Peterson added a comment - It has been brought to my attention that the SQL2003_Standard_Case_Expression.doc is a violation of the ISO copyright, I suggest it be removed as soon as possible. Instead, please refer to http://en.wikipedia.org/wiki/SQL:2003 for a link to a .zip file (sql_2003_standard.zip), and open the 5WD-02-Foundation-2003-09.pdf within. Turn to page 197 (221 of 1332) (Chapter 6.11) to view the Case Expression standard.
          Hide
          Andrew McIntyre added a comment -

          Removed SQL2003_Standard_Case_Expression.doc attachment at attcher's request.

          Show
          Andrew McIntyre added a comment - Removed SQL2003_Standard_Case_Expression.doc attachment at attcher's request.
          Hide
          John Peterson added a comment -

          The org.apache.derby.impl.sql.compile.SQLParser parses NULL's found in SQL case expressions into UntypedNullConstantNode nodes and casts those nodes to the CHAR type. (See SQLParser.caseExpression() on line ~13180).

          The DERBY-7 bug fix took this into account to enable the NULLIF() function to work properly, but this code is only executed if NULLIF() is parsed. (See SQLParser.valueSpecification() on line ~13111). The SQL equivalent to NULLIF(), which is CASE V1=V2 THEN NULL ELSE V1 END, is not flagged to be given this special consideration, and therefore Derby returns the 42X89 error. The error also afflicts the related statements CASE V1=V2 THEN NULL ELSE V3 and CASE V1=V2 THEN V3 ELSE NULL.

          In the attached proposed patch of org.apache.derby.impl.sql.compile.ConditionalNode, Derby will now look more closely at the "then" and "else" nodes during the bindExpression() method. If the node meets the following four conditions, then it is cast to the type of the other node. Otherwise nothing happens, and Derby will proceed as usual.

          1) The "then" or "else" node is a CAST_NODE
          2) The CAST_NODE node operand is an UntypedNullConstantNode
          3) The other node ("else" or "then") has a type service
          4) The "else" and "then" nodes don't have the same type

          These four conditions ensure only NULL's will be cast, and that they'll only be cast if the other node has a type assigned to it that is different than CHAR.

          We have been testing this fix by using our product with Derby, and it appears to be doing the job. I've also executed the derbyall test suite, and the problems which occurred do not seem to stem from this change. The next step is a more extensive testing using all possible database types. I'm planning on moving forward with that, but I wanted to post this patch for review and comment.

          Show
          John Peterson added a comment - The org.apache.derby.impl.sql.compile.SQLParser parses NULL's found in SQL case expressions into UntypedNullConstantNode nodes and casts those nodes to the CHAR type. (See SQLParser.caseExpression() on line ~13180). The DERBY-7 bug fix took this into account to enable the NULLIF() function to work properly, but this code is only executed if NULLIF() is parsed. (See SQLParser.valueSpecification() on line ~13111). The SQL equivalent to NULLIF(), which is CASE V1=V2 THEN NULL ELSE V1 END, is not flagged to be given this special consideration, and therefore Derby returns the 42X89 error. The error also afflicts the related statements CASE V1=V2 THEN NULL ELSE V3 and CASE V1=V2 THEN V3 ELSE NULL. In the attached proposed patch of org.apache.derby.impl.sql.compile.ConditionalNode, Derby will now look more closely at the "then" and "else" nodes during the bindExpression() method. If the node meets the following four conditions, then it is cast to the type of the other node. Otherwise nothing happens, and Derby will proceed as usual. 1) The "then" or "else" node is a CAST_NODE 2) The CAST_NODE node operand is an UntypedNullConstantNode 3) The other node ("else" or "then") has a type service 4) The "else" and "then" nodes don't have the same type These four conditions ensure only NULL's will be cast, and that they'll only be cast if the other node has a type assigned to it that is different than CHAR. We have been testing this fix by using our product with Derby, and it appears to be doing the job. I've also executed the derbyall test suite, and the problems which occurred do not seem to stem from this change. The next step is a more extensive testing using all possible database types. I'm planning on moving forward with that, but I wanted to post this patch for review and comment.
          Hide
          Bernt M. Johnsen added a comment -

          The diff-file contains numerous white-space changes (27 regions out of 29). They should be removed. Otherwise, the patch looks sane to me.

          Show
          Bernt M. Johnsen added a comment - The diff-file contains numerous white-space changes (27 regions out of 29). They should be removed. Otherwise, the patch looks sane to me.
          Hide
          John Peterson added a comment -

          The ConditionalNode_diff.txt is a cleaner diff-file, I've removed the white-space changes.

          Show
          John Peterson added a comment - The ConditionalNode_diff.txt is a cleaner diff-file, I've removed the white-space changes.
          Hide
          Bryan Pendleton added a comment -

          The new version of the diff looks very good, the code reads very well and the comments are
          very well-written and clear.

          The only tiny little confusion I had is over sub-condition (4). In your 3-Nov comment you say:
          4) The "else" and "then" nodes don't have the same type
          and that seems to be what the code implements. But in the code itself your comment says
          4) the other node's type isn't CHAR (avoids duplicate work)
          Can you explain that in just a bit more detail?

          Also, would it be possible for you to include some tests? Perhaps some various examples
          of CASE statements with nulls in various combinations, and also some various
          examples of NULLIF function calls? I see that you indicated in your 3-Nov comment that
          you were working on that; let us know if you've run into any snags or roadblocks.

          Show
          Bryan Pendleton added a comment - The new version of the diff looks very good, the code reads very well and the comments are very well-written and clear. The only tiny little confusion I had is over sub-condition (4). In your 3-Nov comment you say: 4) The "else" and "then" nodes don't have the same type and that seems to be what the code implements. But in the code itself your comment says 4) the other node's type isn't CHAR (avoids duplicate work) Can you explain that in just a bit more detail? Also, would it be possible for you to include some tests? Perhaps some various examples of CASE statements with nulls in various combinations, and also some various examples of NULLIF function calls? I see that you indicated in your 3-Nov comment that you were working on that; let us know if you've run into any snags or roadblocks.
          Hide
          John Peterson added a comment -

          Yes, that comment isn't very clear. I was trying to remind the reader
          that the SQLParser parses NULL's in the CASE expression into
          UntypedNullConstantNode's and then casts those nodes into CHAR's. Thus
          the only situation in which the case statement is returning a CHAR will
          this check need to be performed. Ultimately to avoid having the NULL
          casted to a CHAR for a second time (duplicate work). Thanks for
          pointing it out, I will have to clarify.

          I was working on some test cases, but while writing them I realized that
          patch I provided doesn't handle nested WHEN's. A rather severe
          oversight. So once solution is to recursively examine "else" nodes to
          determine if they are another conditional node. I have been delaying
          updating the code, however, because it's not clear to me as to why Derby
          is casting NULL's into CHAR's in the first place. This question of
          course begs a more detailed look into the underlying Derby code than I
          have yet undergone. In fact, I am planning on posing it to the Derby
          community, for I would be most interested in any feedback.

          Show
          John Peterson added a comment - Yes, that comment isn't very clear. I was trying to remind the reader that the SQLParser parses NULL's in the CASE expression into UntypedNullConstantNode's and then casts those nodes into CHAR's. Thus the only situation in which the case statement is returning a CHAR will this check need to be performed. Ultimately to avoid having the NULL casted to a CHAR for a second time (duplicate work). Thanks for pointing it out, I will have to clarify. I was working on some test cases, but while writing them I realized that patch I provided doesn't handle nested WHEN's. A rather severe oversight. So once solution is to recursively examine "else" nodes to determine if they are another conditional node. I have been delaying updating the code, however, because it's not clear to me as to why Derby is casting NULL's into CHAR's in the first place. This question of course begs a more detailed look into the underlying Derby code than I have yet undergone. In fact, I am planning on posing it to the Derby community, for I would be most interested in any feedback.
          Hide
          John Peterson added a comment -

          A comment in the org.apache.derby.impl.sql.compile.SQLParser.valueSpecification() indicates an assumption that the NULL's in an SQL statement will be taken care of at bind time, from this I'm deducing that it's not the parser's responsibility to ensure NULL's are cast correctly. Perhaps it would be a more complete and efficient solution for the SQLParser to parse NULL's into a currently non-existent NULL type and modify Derby on a larger more complex scale, but I suspect this may not be an ideal solution because there currently doesn't seem to be any demand for a NULL type other than this particular bug. With these thoughts as my motivation, I've created a new patch that will handle NULL's in nested CASE statements. I will provide more extensive tests and their results shortly, however, in the meantime opinions on this are welcome and invited.

          Show
          John Peterson added a comment - A comment in the org.apache.derby.impl.sql.compile.SQLParser.valueSpecification() indicates an assumption that the NULL's in an SQL statement will be taken care of at bind time, from this I'm deducing that it's not the parser's responsibility to ensure NULL's are cast correctly. Perhaps it would be a more complete and efficient solution for the SQLParser to parse NULL's into a currently non-existent NULL type and modify Derby on a larger more complex scale, but I suspect this may not be an ideal solution because there currently doesn't seem to be any demand for a NULL type other than this particular bug. With these thoughts as my motivation, I've created a new patch that will handle NULL's in nested CASE statements. I will provide more extensive tests and their results shortly, however, in the meantime opinions on this are welcome and invited.
          Hide
          A B added a comment -

          John Peterson > I will provide more extensive tests and their results shortly

          John, are you still planning to provide test cases for the changes you have contributed for this issue? And is there anything else that needs to be done here or is the latest ConditionalNode.diff a complete resolution to this problem?

          I looked at what I think is the latest patch (ConditionalNode.diff from 01/09/2007) and it looks pretty good. There are, however, a lot of whitespace diffs in the patch--in fact, if I'm not mistaken the last 160 lines of the patch are whitespace-only changes? It'd be great if those could be removed.

          If you are still planning to contribtue test cases and there is nothing else to do for his issue (aside from whitespace cleanup), I'd be willing to commit your changes to the Derby trunk for you. Please post a comment if you are still interested.
          Thank you for your time on this!

          Show
          A B added a comment - John Peterson > I will provide more extensive tests and their results shortly John, are you still planning to provide test cases for the changes you have contributed for this issue? And is there anything else that needs to be done here or is the latest ConditionalNode.diff a complete resolution to this problem? I looked at what I think is the latest patch (ConditionalNode.diff from 01/09/2007) and it looks pretty good. There are, however, a lot of whitespace diffs in the patch--in fact, if I'm not mistaken the last 160 lines of the patch are whitespace-only changes? It'd be great if those could be removed. If you are still planning to contribtue test cases and there is nothing else to do for his issue (aside from whitespace cleanup), I'd be willing to commit your changes to the Derby trunk for you. Please post a comment if you are still interested. Thank you for your time on this!
          Hide
          John Peterson added a comment -

          The latest patch (March 12, 2007) fixes the two problems my last patch still had issues with.

          1) NULL's inside the THEN braches of the CASE Expressions where not being cast. Causing the exception to be thrown.
          (e.g. VALUES CASE WHEN 1 = 1 THEN (CASE WHEN 1 = 1 THEN NULL ELSE 1 END) ELSE NULL END

          2) Column References were ignored. Causing the exception to be thrown.
          (e.g. SELECT Count(CASE WHEN 1 = 1 THEN aDecimalColumnName ELSE NULL END) FROM aTableName

          I have done extensive testing, and I believe this patch finally fixes this bug. Therefore, once these changes are commited, I will close out this bug.

          Thanks,
          John

          Show
          John Peterson added a comment - The latest patch (March 12, 2007) fixes the two problems my last patch still had issues with. 1) NULL's inside the THEN braches of the CASE Expressions where not being cast. Causing the exception to be thrown. (e.g. VALUES CASE WHEN 1 = 1 THEN (CASE WHEN 1 = 1 THEN NULL ELSE 1 END) ELSE NULL END 2) Column References were ignored. Causing the exception to be thrown. (e.g. SELECT Count(CASE WHEN 1 = 1 THEN aDecimalColumnName ELSE NULL END) FROM aTableName I have done extensive testing, and I believe this patch finally fixes this bug. Therefore, once these changes are commited, I will close out this bug. Thanks, John
          Hide
          A B added a comment -

          > I have done extensive testing, and I believe this patch finally fixes this bug.

          That's great to hear. Can I ask what you mean by "extensive testing"? Did you run the Derby regression suites at all? Do you have a list of queries that you ran to verify everything is working as it should? And if so, is it possible to add those queries to one of Derby's existing regression tests? This allows other developers to ensure that everything will continue to work in the future.

          If you do not feel like incorporating your test queries into an existing (or new) regression test, then it would be great if you could at least post the queries (ex. as an ij script) to this issue so that any other developers who may be interested can add the test cases to the regression suite. It would, of course, be wonderful if you were willing and able to do that yourself.

          I as a committer am a bit hesitant to commit changes for which no new regression tests have been added. There are exceptions, of course, but in this particular case I think it would be great to have some new test cases running every night to verify the fix. And if you've already done "extensive testing", perhaps that means you have such test cases already written...?

          Show
          A B added a comment - > I have done extensive testing, and I believe this patch finally fixes this bug. That's great to hear. Can I ask what you mean by "extensive testing"? Did you run the Derby regression suites at all? Do you have a list of queries that you ran to verify everything is working as it should? And if so, is it possible to add those queries to one of Derby's existing regression tests? This allows other developers to ensure that everything will continue to work in the future. If you do not feel like incorporating your test queries into an existing (or new) regression test, then it would be great if you could at least post the queries (ex. as an ij script) to this issue so that any other developers who may be interested can add the test cases to the regression suite. It would, of course, be wonderful if you were willing and able to do that yourself. I as a committer am a bit hesitant to commit changes for which no new regression tests have been added. There are exceptions, of course, but in this particular case I think it would be great to have some new test cases running every night to verify the fix. And if you've already done "extensive testing", perhaps that means you have such test cases already written...?
          Hide
          John Peterson added a comment -

          I did run the entire test suite against it, and none of the problems
          that occurred appeared to stem from the modification of the
          ConditionalNode class. My "extensive" testing can not be shared as
          they are a set of proprietary queries not written by me against a
          database not created by me. You do make an excellent point, however, so
          I'll write up a test suite that takes advantage of the existing tests
          put into place by the author of the DERBY-7 fix. I'll shoot for this
          week.

          John

          Show
          John Peterson added a comment - I did run the entire test suite against it, and none of the problems that occurred appeared to stem from the modification of the ConditionalNode class. My "extensive" testing can not be shared as they are a set of proprietary queries not written by me against a database not created by me. You do make an excellent point, however, so I'll write up a test suite that takes advantage of the existing tests put into place by the author of the DERBY-7 fix. I'll shoot for this week. John
          Hide
          John Peterson added a comment -

          The results of the test indicate that problems occur when trying to cast the casted NULL into something that can not be converted from a CHAR, (e.g. From line 2090 of resultset.tmp:

          SELECT CASE WHEN 1 = 1 THEN REALCOL ELSE NULL END from AllDataTypesTable
          ERROR 42846: Cannot convert types 'CHAR' to 'REAL'. )

          Even though the statement is no longer issuing the 42X89 error, it's still not up to SQL Standard. Since I am unsure of the impact of changing the SQLParser (which sets NULL's to type CHAR), I'm going to change the cast type from CHAR to REAL instead of casting the casted NULL to a REAL (causing the error). The good news is that this will be a substantial improvement for anyone who wants to use NULL's with a type that can't be casted from a CHAR in a CASE Expression, for which there is currently no workaround.
          John

          Show
          John Peterson added a comment - The results of the test indicate that problems occur when trying to cast the casted NULL into something that can not be converted from a CHAR, (e.g. From line 2090 of resultset.tmp: SELECT CASE WHEN 1 = 1 THEN REALCOL ELSE NULL END from AllDataTypesTable ERROR 42846: Cannot convert types 'CHAR' to 'REAL'. ) Even though the statement is no longer issuing the 42X89 error, it's still not up to SQL Standard. Since I am unsure of the impact of changing the SQLParser (which sets NULL's to type CHAR), I'm going to change the cast type from CHAR to REAL instead of casting the casted NULL to a REAL (causing the error). The good news is that this will be a substantial improvement for anyone who wants to use NULL's with a type that can't be casted from a CHAR in a CASE Expression, for which there is currently no workaround. John
          Hide
          John Peterson added a comment -

          The latest patch, test code, and test results. The latest change was a simple one. If required, the NULL is relieved of it's CHAR association and recast to a more appropriate type.

          Show
          John Peterson added a comment - The latest patch, test code, and test results. The latest change was a simple one. If required, the NULL is relieved of it's CHAR association and recast to a more appropriate type.
          Hide
          A B added a comment -

          Thank you for your continued work on this, John. I'm hoping to review and commit the latest patch sometime in the next couple of days.

          One thing I did notice: We have been trying to move as many of the regression tests as possible into JUnit :

          http://wiki.apache.org/db-derby/KillDerbyTestHarness
          http://wiki.apache.org/db-derby/DerbyJUnitTesting

          As part of that effort the jdbcapi/resultset.java test was recently converted (just a couple of days ago, actually), and thus that particular test file no longer exists--see DERBY-2429. So the test patch that you have won't actually apply to the latest codeline

          My apologies here: I should have mentioned the preference for JUnit tests when I asked earlier, and I didn't realize that the test you were changing was going to be converted this week. Ack.

          So that said, do you have any interest in porting your new test cases to JUnit based on the new JUnit tests added as part of DERBY-2429? I understand if you find that to be a bit too annoying; if so, we at least have the resultset.java changes (thank you!) so anyone else in the community who is so inclined can make the conversion. But I thought I'd ask just to see...

          Thank you very much for your continued patience with this effort. As I said, I'll try to review/convert the engine changes over the next couple of days (maybe early next week if I can't get to it before then).

          Show
          A B added a comment - Thank you for your continued work on this, John. I'm hoping to review and commit the latest patch sometime in the next couple of days. One thing I did notice: We have been trying to move as many of the regression tests as possible into JUnit : http://wiki.apache.org/db-derby/KillDerbyTestHarness http://wiki.apache.org/db-derby/DerbyJUnitTesting As part of that effort the jdbcapi/resultset.java test was recently converted (just a couple of days ago, actually), and thus that particular test file no longer exists--see DERBY-2429 . So the test patch that you have won't actually apply to the latest codeline My apologies here: I should have mentioned the preference for JUnit tests when I asked earlier, and I didn't realize that the test you were changing was going to be converted this week. Ack. So that said, do you have any interest in porting your new test cases to JUnit based on the new JUnit tests added as part of DERBY-2429 ? I understand if you find that to be a bit too annoying; if so, we at least have the resultset.java changes (thank you!) so anyone else in the community who is so inclined can make the conversion. But I thought I'd ask just to see... Thank you very much for your continued patience with this effort. As I said, I'll try to review/convert the engine changes over the next couple of days (maybe early next week if I can't get to it before then).
          Hide
          John Peterson added a comment -

          Of course, I'll be happy to.
          John

          Show
          John Peterson added a comment - Of course, I'll be happy to. John
          Hide
          A B added a comment -

          Hi John, just a few minor comments on the most recent patch:

          • The following two methods do not have any javadoc; it's not that big of a
            deal since it's pretty clear that what they are doing--but a line or two
            of javadoc content wouldn't hurt...
          • isCastNode(ValueNode)
          • isCastToChar(CastNode)
          • There are several un-used versions of "shouldCast()" in this patch: the
            only one that is directly called is (DataTypeDescriptor, ValueNode), and
            that simply calls the version which takes two DTDs. So I wonder if we could
            just have the single version that takes two DTDs and then call that
            directly, ex:

          Instead of:

          if (isNullNode(thenNode) && shouldCast(castType, thenNode)) {

          just do:

          if (isNullNode(thenNode) &&
          shouldCast(castType, thenNode.getTypeServices()))
          {

          Then you could remove all of the other versions of the method.

          • The sequential "if" statements in the new "findType()" method are a tad
            hard to follow. If we remove the underlying code blocks we end up with
            something like:

          + if ((thenType != null) && !isCastNode(thenNode) && !isConditionalNode(thenNode))
          + if (isCastNode(thenNode) && !isCastToChar((CastNode)thenNode))
          + if ((elseType != null) && !isCastNode(elseNode) && !isConditionalNode(elseNode))
          + if (isCastNode(elseNode) && !isCastToChar((CastNode)elseNode))
          + if (isConditionalNode(thenNode))
          + if (theType != null)
          + if (isConditionalNode(elseNode))
          + if (theType != null)

          Maybe I'm just being paranoid, but when I see code like this I have to
          stare for a long time to convince myself that all of the different paths
          have been covered. It's especially tough given that certain conditions (ex.
          "isCastNode(thenNode)") show up more than once.

          Of course, you added good comments to this code which were very helpful--so
          thank you for that! I don't think you need to change anything here, but since
          I spent a good deal of time scratching my head and trying to convince myself
          that all of this is correct, I thought I'd mention it...

          But these are all minor things that can be addressed with a follow-up patch if you are so inclined.

          I ran derbyall and suites.All on Red Hat Linux with ibm142 and there were no failures, so I made some minor formatting changes to the patch (lines longer than 80 chars--it'd be great if these could be avoided in the future...) and committed the engine changes with svn #520173:

          URL: http://svn.apache.org/viewvc?view=rev&rev=520173

          Since you have kindly agreed to work on rewriting the tests for JUnit (thank you!) I did not commit the "resultset" changes and am leaving the issue 'Open' for now...

          Thank you for the contribution!

          Show
          A B added a comment - Hi John, just a few minor comments on the most recent patch: The following two methods do not have any javadoc; it's not that big of a deal since it's pretty clear that what they are doing--but a line or two of javadoc content wouldn't hurt... isCastNode(ValueNode) isCastToChar(CastNode) There are several un-used versions of "shouldCast()" in this patch: the only one that is directly called is (DataTypeDescriptor, ValueNode), and that simply calls the version which takes two DTDs. So I wonder if we could just have the single version that takes two DTDs and then call that directly, ex: Instead of: if (isNullNode(thenNode) && shouldCast(castType, thenNode)) { just do: if (isNullNode(thenNode) && shouldCast(castType, thenNode.getTypeServices())) { Then you could remove all of the other versions of the method. The sequential "if" statements in the new "findType()" method are a tad hard to follow. If we remove the underlying code blocks we end up with something like: + if ((thenType != null) && !isCastNode(thenNode) && !isConditionalNode(thenNode)) + if (isCastNode(thenNode) && !isCastToChar((CastNode)thenNode)) + if ((elseType != null) && !isCastNode(elseNode) && !isConditionalNode(elseNode)) + if (isCastNode(elseNode) && !isCastToChar((CastNode)elseNode)) + if (isConditionalNode(thenNode)) + if (theType != null) + if (isConditionalNode(elseNode)) + if (theType != null) Maybe I'm just being paranoid, but when I see code like this I have to stare for a long time to convince myself that all of the different paths have been covered. It's especially tough given that certain conditions (ex. "isCastNode(thenNode)") show up more than once. Of course, you added good comments to this code which were very helpful--so thank you for that! I don't think you need to change anything here, but since I spent a good deal of time scratching my head and trying to convince myself that all of this is correct, I thought I'd mention it... But these are all minor things that can be addressed with a follow-up patch if you are so inclined. I ran derbyall and suites.All on Red Hat Linux with ibm142 and there were no failures, so I made some minor formatting changes to the patch (lines longer than 80 chars--it'd be great if these could be avoided in the future...) and committed the engine changes with svn #520173: URL: http://svn.apache.org/viewvc?view=rev&rev=520173 Since you have kindly agreed to work on rewriting the tests for JUnit (thank you!) I did not commit the "resultset" changes and am leaving the issue 'Open' for now... Thank you for the contribution!
          Hide
          A B added a comment -

          Oops, somehow I unassigned John Peterson from this issue (sorry!). I tried to reassign it back to him but he doesn't show up on the available list. John, please re-assign this issue back to you when you have the chance. Sorry for the slip-up...not sure how that happened...

          Show
          A B added a comment - Oops, somehow I unassigned John Peterson from this issue (sorry!). I tried to reassign it back to him but he doesn't show up on the available list. John, please re-assign this issue back to you when you have the chance. Sorry for the slip-up...not sure how that happened...
          Hide
          Andrew McIntyre added a comment -

          Reassign John Peterson to this issue. Needed to update his JIRA user profile to be a contributor to the derby-developers group.

          Show
          Andrew McIntyre added a comment - Reassign John Peterson to this issue. Needed to update his JIRA user profile to be a contributor to the derby-developers group.
          Hide
          John Peterson added a comment -

          Per previous comments, I've added javadoc information to the private methods isCastNode() and isCastToChar(), removed the unused shouldCast() methods, updated the recastNullNodes to use the remaining shouldCast() method, and tried to improve the comments on the if-then-else blocks determining recursive calls. I've also created a new class org.apache.derbyTesting.functionTests.tests.lang.CaseExpressionTest.java which converts my additions to the org.apache.derbyTesting.functionTests.tests.jdbcapi.resultset.java class over to the new junit testing suite. If these last few small changes are acceptable, then I will close out this bug.

          Show
          John Peterson added a comment - Per previous comments, I've added javadoc information to the private methods isCastNode() and isCastToChar(), removed the unused shouldCast() methods, updated the recastNullNodes to use the remaining shouldCast() method, and tried to improve the comments on the if-then-else blocks determining recursive calls. I've also created a new class org.apache.derbyTesting.functionTests.tests.lang.CaseExpressionTest.java which converts my additions to the org.apache.derbyTesting.functionTests.tests.jdbcapi.resultset.java class over to the new junit testing suite. If these last few small changes are acceptable, then I will close out this bug.
          Hide
          A B added a comment -

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

          Show
          A B added a comment - 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...
          Hide
          John Peterson added a comment -

          I've changed the comment headers to indicate this class is CaseExpressionTest, removed the unnecessary imports, removed the "System.out" statements (which are not needed), added the test to the lang/_Suite class, and renamed this patch from "ConditionalNode.diff" to "derby1620_test.patch" for clarification. I've also updated the suite() method by removing the client/server test, and removing the call to the local baseSuite() method which was clearly confusing.

          Show
          John Peterson added a comment - I've changed the comment headers to indicate this class is CaseExpressionTest, removed the unnecessary imports, removed the "System.out" statements (which are not needed), added the test to the lang/_Suite class, and renamed this patch from "ConditionalNode.diff" to "derby1620_test.patch" for clarification. I've also updated the suite() method by removing the client/server test, and removing the call to the local baseSuite() method which was clearly confusing.
          Hide
          A B added a comment -

          Thank you for the revised patch, John. I applied it to my codeline and ran the regression tests (derbyall and suites.All) as a sanity check. I then did some cleanup of the new CaseExpressionTest to a) keep lines under 80 characters, and b) move common code into a single testCaseExpressionQuery() method, to avoid code duplication. I also verified that the new test fails if the fix for this issue is reverted, and passes with the fix applied. So everything looks good.

          Committed derby1620_test_v2.patch (which has the changes I mentioned above) with svn # 544974:

          URL: http://svn.apache.org/viewvc?view=rev&rev=544974

          Marking issue as resolved in 10.3. I'll leave it up to you close it if everything looks okay.

          Thanks for fixing this bug!

          Show
          A B added a comment - Thank you for the revised patch, John. I applied it to my codeline and ran the regression tests (derbyall and suites.All) as a sanity check. I then did some cleanup of the new CaseExpressionTest to a) keep lines under 80 characters, and b) move common code into a single testCaseExpressionQuery() method, to avoid code duplication. I also verified that the new test fails if the fix for this issue is reverted, and passes with the fix applied. So everything looks good. Committed derby1620_test_v2.patch (which has the changes I mentioned above) with svn # 544974: URL: http://svn.apache.org/viewvc?view=rev&rev=544974 Marking issue as resolved in 10.3. I'll leave it up to you close it if everything looks okay. Thanks for fixing this bug!

            People

            • Assignee:
              John Peterson
              Reporter:
              John Peterson
            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development