Derby
  1. Derby
  2. DERBY-4553

In ij GETCURRENTROWNUMBER directly writeits result to output

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: Tools
    • Labels:
    • Issue & fix info:
      Newcomer

      Description

      In ij, the statement GETCURRENTROWNUMBER directly write its result to output instead of returning it:

      Here are the faulty lines in ij.ij, method GetCurrentRowNumber():
      ...
      LocalizedResource.OutputWriter().println(utilInstance.getCurrentRowNumber(rs));
      return null;

      This interferes with testing - and possibly with any external tool using the ij.ij parser.

      1. DERBY-4553_2.patch
        3 kB
        Sylvain Leroux
      2. DERBY-4553_repro.patch
        1 kB
        Sylvain Leroux
      3. DERBY-4553.patch
        4 kB
        Sylvain Leroux

        Activity

        Hide
        Sylvain Leroux added a comment -

        Attaching a repro for this issue.

        The repro just create a cursor and add a call to GETCURRENTROWNUMBER at the end of ij7.sql. Running the ToolScripts test suite produce the following result:
        sh$ ant all && java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.tools.ToolScripts
        [snip]
        .0
        F....0
        F...
        Time: 130.112
        There were 2 failures:
        [snip]

        The '0's in the output are the result of GETCURRENTROWNUMBER. And are not caught by the test tool. That leads to a failure of the corresponding test.


        By looking at the source of ij, it seems there is currently no class implementing ijResult suitable to return only one value. An option would be to create a new class derived of ijResultVector to implement scalar results as 1-dimension vector.

        Show
        Sylvain Leroux added a comment - Attaching a repro for this issue. The repro just create a cursor and add a call to GETCURRENTROWNUMBER at the end of ij7.sql. Running the ToolScripts test suite produce the following result: sh$ ant all && java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.tools.ToolScripts [snip] .0 F....0 F... Time: 130.112 There were 2 failures: [snip] The '0's in the output are the result of GETCURRENTROWNUMBER. And are not caught by the test tool. That leads to a failure of the corresponding test. By looking at the source of ij, it seems there is currently no class implementing ijResult suitable to return only one value. An option would be to create a new class derived of ijResultVector to implement scalar results as 1-dimension vector.
        Hide
        Sylvain Leroux added a comment -

        Attaching a patch for this issue. Ready for review.

        This patch introduce the classe ijScalarResult representing a single value returned from an ij statement. ijScalarResult is defined as a subclass of ijVectorResult. I use it to store the integer value returned by getCurrentRowNumber().

        However, according to the comment on ijVectorResult, that class is for vectors of /strings/. But, the only usage for such vectors ends up in org.apache.derby.impl.tools.ij.util.DisplayVector(LocalizedOutput, Vector) where a println is issued for each item. So, I don't think it is a problem to feed such a vector with other kind of objects. Moreover, ijVectorResult is package-private so there is little chance this break anything outside of ij.

        I wasn't able to find where ij's cursor statements are tested (are they?). So I add a simple test for GETCURRENTROWNUMBER in ij7.sql

        Pass ToolScript test suite:
        sh$ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.tools.ToolScripts
        ........
        Time: 24.031

        OK (8 tests)

        Show
        Sylvain Leroux added a comment - Attaching a patch for this issue. Ready for review. This patch introduce the classe ijScalarResult representing a single value returned from an ij statement. ijScalarResult is defined as a subclass of ijVectorResult. I use it to store the integer value returned by getCurrentRowNumber(). However, according to the comment on ijVectorResult, that class is for vectors of /strings/. But, the only usage for such vectors ends up in org.apache.derby.impl.tools.ij.util.DisplayVector(LocalizedOutput, Vector) where a println is issued for each item. So, I don't think it is a problem to feed such a vector with other kind of objects. Moreover, ijVectorResult is package-private so there is little chance this break anything outside of ij. I wasn't able to find where ij's cursor statements are tested (are they?). So I add a simple test for GETCURRENTROWNUMBER in ij7.sql Pass ToolScript test suite: sh$ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.tools.ToolScripts ........ Time: 24.031 OK (8 tests)
        Hide
        Knut Anders Hatlen added a comment -

        Hi Sylvain,

        The fix looks fine to me, but I think we need to update the comments if we start allowing other objects than strings in ijVectorResult. I'm wondering, though, if we need the new class, or if it would be better to just add an extra constructor to ijVectorResult that takes a string argument. Not a big issue, but that would give slightly less code and less files to maintain.

        (On a side note, this would not have been an issue if only ijVectorResult had used java.util.List instead of java.util.Vector. Then we could produce a scalar result as simple as this, without any new class or new constructor: "new ijVectorResult(Collections.singletonList(result)), null);" But cleaning up the ij code and making it use the more modern collection classes is outside the scope of this issue.)

        Show
        Knut Anders Hatlen added a comment - Hi Sylvain, The fix looks fine to me, but I think we need to update the comments if we start allowing other objects than strings in ijVectorResult. I'm wondering, though, if we need the new class, or if it would be better to just add an extra constructor to ijVectorResult that takes a string argument. Not a big issue, but that would give slightly less code and less files to maintain. (On a side note, this would not have been an issue if only ijVectorResult had used java.util.List instead of java.util.Vector. Then we could produce a scalar result as simple as this, without any new class or new constructor: "new ijVectorResult(Collections.singletonList(result)), null);" But cleaning up the ij code and making it use the more modern collection classes is outside the scope of this issue.)
        Hide
        Sylvain Leroux added a comment - - edited

        Hi Knut,

        Thanks for reviewing such a critical issue

        I attach here a new patch for this issue:

        • I agree with you to add extra constructors to ijVectorResult instead of coding a new class.
        • I have updated the comment about ijVectorResult accepting any object.

        Pass derbytools ant ToolScripts test suite.

        Concerning your side note, I think ij deserves some cleaning up. Both at source level - and maybe as well in terms of functionality? According to a comment in java/tools/org/apache/derby/impl/tools/ij/Main.java, it was first designed as a testing tool. But ij could have some "advanced" feature to ease database maintenance tasks.

        Show
        Sylvain Leroux added a comment - - edited Hi Knut, Thanks for reviewing such a critical issue I attach here a new patch for this issue: I agree with you to add extra constructors to ijVectorResult instead of coding a new class. I have updated the comment about ijVectorResult accepting any object. Pass derbytools ant ToolScripts test suite. Concerning your side note, I think ij deserves some cleaning up. Both at source level - and maybe as well in terms of functionality? According to a comment in java/tools/org/apache/derby/impl/tools/ij/Main.java, it was first designed as a testing tool. But ij could have some "advanced" feature to ease database maintenance tasks.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Sylvain! I'll run the full regression test suite and commit the patch if nothing breaks.

        Show
        Knut Anders Hatlen added a comment - Thanks, Sylvain! I'll run the full regression test suite and commit the patch if nothing breaks.
        Hide
        Knut Anders Hatlen added a comment -

        The tests ran cleanly in my environment. Committed revision 911952.

        Show
        Knut Anders Hatlen added a comment - The tests ran cleanly in my environment. Committed revision 911952.

          People

          • Assignee:
            Sylvain Leroux
            Reporter:
            Sylvain Leroux
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development