Derby
  1. Derby
  2. DERBY-3842

Convert "org.apache.derbyTesting.functionTests.tests.store.holdCursorExternalSortJDBC30.sql" to junit.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: Test
    • Labels:
      None
    1. derby-3842-1.patch
      22 kB
      Junjie Peng
    2. derby-3842-1.stat
      0.7 kB
      Junjie Peng
    3. derby-3842-tiago.patch
      54 kB
      Tiago R. Espinha
    4. derby-3842-tiago.patch
      56 kB
      Tiago R. Espinha

      Issue Links

        Activity

        Hide
        Tiago R. Espinha added a comment -

        The test was converted and the patch committed.

        Show
        Tiago R. Espinha added a comment - The test was converted and the patch committed.
        Hide
        Kathey Marsden added a comment -

        Committed revision 760489.

        Show
        Kathey Marsden added a comment - Committed revision 760489.
        Hide
        Kathey Marsden added a comment -

        Thanks Knut and Tiago for the clarification on SanityManager.DEBUG. I guess it just gets compiled away. I'll go ahead and commit the patch.

        Show
        Kathey Marsden added a comment - Thanks Knut and Tiago for the clarification on SanityManager.DEBUG. I guess it just gets compiled away. I'll go ahead and commit the patch.
        Hide
        Knut Anders Hatlen added a comment -

        I think you're right that it's OK with SanityManager.DEBUG. I haven't checked, but I think it's used in some other tests as well without causing any problems. And the use of SanityManager.DEBUG doesn't cause any problems in the production code, even if the SanityManager class doesn't exist in the insane jars.

        Show
        Knut Anders Hatlen added a comment - I think you're right that it's OK with SanityManager.DEBUG. I haven't checked, but I think it's used in some other tests as well without causing any problems. And the use of SanityManager.DEBUG doesn't cause any problems in the production code, even if the SanityManager class doesn't exist in the insane jars.
        Hide
        Tiago R. Espinha added a comment -

        Both suites.All and derbyall pass on both sane and insane mode. I think it's safe to say that the check is working properly?

        Show
        Tiago R. Espinha added a comment - Both suites.All and derbyall pass on both sane and insane mode. I think it's safe to say that the check is working properly?
        Hide
        Tiago R. Espinha added a comment -

        I am fairly sure that the SanityManager.DEBUG works on insane builds and in that case, it just is set to false.

        I say this because I took the liberty of running just this suite against an insane build and the test was correctly skipped (I added some output to the test's constructor and this output would show with a sane build but not with an insane one).

        I am nonetheless running the whole derbyall and suites.All on an insane build just to be sure. Both suites did however pass on a sane build (after that little hiccup on derbyall last night, which as you suggested I'll post as an issue).

        Show
        Tiago R. Espinha added a comment - I am fairly sure that the SanityManager.DEBUG works on insane builds and in that case, it just is set to false. I say this because I took the liberty of running just this suite against an insane build and the test was correctly skipped (I added some output to the test's constructor and this output would show with a sane build but not with an insane one). I am nonetheless running the whole derbyall and suites.All on an insane build just to be sure. Both suites did however pass on a sane build (after that little hiccup on derbyall last night, which as you suggested I'll post as an issue).
        Hide
        Kathey Marsden added a comment - - edited

        The patch looks good to me except I am not sure the if (SanityManager.DEBUG) in store._Suite is going to work with an insane build, because the SanityManager class is not present at all.

        I think you will have to use reflection to determine if this is a sane build and perhaps the best thing to do is put an isSane() method in TestConfiguration so it can be used by others if needed in the future.

        Then I don't know if it would be worthwhile to keep the test running for insane builds and then just put the isSane() condition around the RuntimeStatisticsParser call or if the test is just of no use in insane mode.

        Show
        Kathey Marsden added a comment - - edited The patch looks good to me except I am not sure the if (SanityManager.DEBUG) in store._Suite is going to work with an insane build, because the SanityManager class is not present at all. I think you will have to use reflection to determine if this is a sane build and perhaps the best thing to do is put an isSane() method in TestConfiguration so it can be used by others if needed in the future. Then I don't know if it would be worthwhile to keep the test running for insane builds and then just put the isSane() condition around the RuntimeStatisticsParser call or if the test is just of no use in insane mode.
        Hide
        Tiago R. Espinha added a comment -

        New patch.

        Quick overview:

        • Added the test to the Suite, on the condition that it is a sane build. (Added this condition to the _Suite file, since the whole test should not be added in case it's an insane environment).
        • Removed the following files:
          -> holdCursorExternalSortJDBC30_app.properties
          -> holdCursorExternalSortJDBC30.sql
          -> holdCursorExternalSortJDBC30_derby.properties
          -> holdCursorExternalSortJDBC30.out
        • Implemented Kathey's suggestions
        • Added a method for checking whether an External Sort was used or not on the RuntimeStatisticsParser. Also added a toString() override on this class, that allow us to have access to the complete output of the runtime statistics.
        • The tests do check now whether an External Sort was used or not by checking the statistics.

        And I think that's all.

        Show
        Tiago R. Espinha added a comment - New patch. Quick overview: Added the test to the Suite, on the condition that it is a sane build. (Added this condition to the _Suite file, since the whole test should not be added in case it's an insane environment). Removed the following files: -> holdCursorExternalSortJDBC30_app.properties -> holdCursorExternalSortJDBC30.sql -> holdCursorExternalSortJDBC30_derby.properties -> holdCursorExternalSortJDBC30.out Implemented Kathey's suggestions Added a method for checking whether an External Sort was used or not on the RuntimeStatisticsParser. Also added a toString() override on this class, that allow us to have access to the complete output of the runtime statistics. The tests do check now whether an External Sort was used or not by checking the statistics. And I think that's all.
        Hide
        Kristian Waagan added a comment -

        I second Bryan's comment.
        At the same time I would like to reiterate that tests should be written to be independent of whether Derby has been built in sane or insane mode if possible.
        As a related issue, writing package private tests should also be avoided if feasible. I believe these tests are being run by less people than the other tests.

        Show
        Kristian Waagan added a comment - I second Bryan's comment. At the same time I would like to reiterate that tests should be written to be independent of whether Derby has been built in sane or insane mode if possible. As a related issue, writing package private tests should also be avoided if feasible. I believe these tests are being run by less people than the other tests.
        Hide
        Bryan Pendleton added a comment -

        I think it's OK to have a test which only runs in SANE mode. That's better than not
        having a test at all, in my opinion. And I think that using these special internal
        properties to force the test through the code path that we want it to take is fine,
        because it is after all a test, and is intending to test that particular behavior.

        So I think you should leave your test as is, using the special properties, but
        adjust it so that the test checks to see whether this is a SANE or INSANE build,
        and skips the test in the INSANE configuration, and you should submit your patch.

        It will be good to have a test which we know is reliably forcing an external sort,
        because we need to have coverage of the external sort behaviors and this is one
        way to do that.

        Show
        Bryan Pendleton added a comment - I think it's OK to have a test which only runs in SANE mode. That's better than not having a test at all, in my opinion. And I think that using these special internal properties to force the test through the code path that we want it to take is fine, because it is after all a test, and is intending to test that particular behavior. So I think you should leave your test as is, using the special properties, but adjust it so that the test checks to see whether this is a SANE or INSANE build, and skips the test in the INSANE configuration, and you should submit your patch. It will be good to have a test which we know is reliably forcing an external sort, because we need to have coverage of the external sort behaviors and this is one way to do that.
        Hide
        Tiago R. Espinha added a comment -

        I was just looking at ExternalSortFactory and MergeInserter and I think sortBufferMax doesn't really matter at all unless we have the debug on. And it seems like this is an expected behaviour.

        Maybe this parameter isn't intended for actual use and it's just there for testing sake? With the debug set to true the tests do use an external sort 100% of the times...

        And then there's this comment on ExternalSortFactory:
        // RESOLVE - mikem change this to use estimatedRows and
        // estimatedRowSize to come up with a smarter number for sortBufferMax
        // than a fixed number of rows. At least 2 possibilities:
        // 1) add sortBufferMaxMem which would be the amount of memory
        // the sorter could use, and then just pick the number of
        // rows as (sortBufferMaxMem / (estimatedRows * estimatedRowSize)
        // 2) add sortBufferUsePercentFree. This would be how much of
        // the current free memory can the current sort use.
        //

        which I think further supports my theory. Maybe it's not wise to let the user choose a maximum value for their sortBuffer on a row-amount basis?

        Also, how should I go regarding the test? Do I leave it with the debug parameter set and just submit a patch?

        Show
        Tiago R. Espinha added a comment - I was just looking at ExternalSortFactory and MergeInserter and I think sortBufferMax doesn't really matter at all unless we have the debug on. And it seems like this is an expected behaviour. Maybe this parameter isn't intended for actual use and it's just there for testing sake? With the debug set to true the tests do use an external sort 100% of the times... And then there's this comment on ExternalSortFactory: // RESOLVE - mikem change this to use estimatedRows and // estimatedRowSize to come up with a smarter number for sortBufferMax // than a fixed number of rows. At least 2 possibilities: // 1) add sortBufferMaxMem which would be the amount of memory // the sorter could use, and then just pick the number of // rows as (sortBufferMaxMem / (estimatedRows * estimatedRowSize) // 2) add sortBufferUsePercentFree. This would be how much of // the current free memory can the current sort use. // which I think further supports my theory. Maybe it's not wise to let the user choose a maximum value for their sortBuffer on a row-amount basis? Also, how should I go regarding the test? Do I leave it with the debug parameter set and just submit a patch?
        Hide
        Kathey Marsden added a comment -

        I am not sure about the intermittent nature, but there does seem to be some code that makes real changes based on testSort:
        In MergeInserter we have

        if (SanityManager.DEBUG)
        {
        if (SanityManager.DEBUG_ON("testSort"))

        { avoidMergeRun = false; }

        }

        and in MergeSort we have:
        if (SanityManager.DEBUG_ON("testSort"))
        sortBufferMin = sortBufferMax;

        It would be good if someone can shed some light on what is the intended use of derby.debug.true=testSort and whether it should affect derby.storage.sortBufferMax.

        Show
        Kathey Marsden added a comment - I am not sure about the intermittent nature, but there does seem to be some code that makes real changes based on testSort: In MergeInserter we have if (SanityManager.DEBUG) { if (SanityManager.DEBUG_ON("testSort")) { avoidMergeRun = false; } } and in MergeSort we have: if (SanityManager.DEBUG_ON("testSort")) sortBufferMin = sortBufferMax; It would be good if someone can shed some light on what is the intended use of derby.debug.true=testSort and whether it should affect derby.storage.sortBufferMax.
        Hide
        Tiago R. Espinha added a comment -

        You're right Kathey. I have built the code and jars in insane mode and the test fails.

        Just some more info that I have been able to acquire through testing:

        • The sortBufferMax is being set now. I have been able to confirm this through direct output from the ExternalSortFactory.java and also through the getSystemProperty method. This value is being set to 5 like it should.
        • When I'm running an insane build, or if I simply omit the derby.debug.true, the tests passing or failing is seemingly random. They fail most of the times but randomly one of them will pass, which I am truly clueless about. I have even set up the test so that it creates two separate tables now, since there's a test that requires additional data to be added.

        When the said tests fail though, I have confirmed that the sortBufferMax is indeed still set to 5, but the statistics just show that an internal sort was done rather than an external one. Weirdly enough, if the debug is set to true, the tests pass all the time. I even made several consecutive runs of the test just to be sure and it is a 100% pass rate.

        Does anyone have any ideas as to why this is happening?

        Show
        Tiago R. Espinha added a comment - You're right Kathey. I have built the code and jars in insane mode and the test fails. Just some more info that I have been able to acquire through testing: The sortBufferMax is being set now. I have been able to confirm this through direct output from the ExternalSortFactory.java and also through the getSystemProperty method. This value is being set to 5 like it should. When I'm running an insane build, or if I simply omit the derby.debug.true, the tests passing or failing is seemingly random. They fail most of the times but randomly one of them will pass, which I am truly clueless about. I have even set up the test so that it creates two separate tables now, since there's a test that requires additional data to be added. When the said tests fail though, I have confirmed that the sortBufferMax is indeed still set to 5, but the statistics just show that an internal sort was done rather than an external one. Weirdly enough, if the debug is set to true, the tests pass all the time. I even made several consecutive runs of the test just to be sure and it is a 100% pass rate. Does anyone have any ideas as to why this is happening?
        Hide
        Kathey Marsden added a comment -

        I don't think derby.debug.true=testSort should be required for derby.storage.sortBufferMax to work. If that's the case, we should probably file a bug. Make it minor since I don't immediately see derby.storage.sortBufferMax documented.

        Also if you are setting it in your test to work around the issue, check your test and see if it works for an insane build. Typically the derby.debug.* code is not used for insane builds.

        Show
        Kathey Marsden added a comment - I don't think derby.debug.true=testSort should be required for derby.storage.sortBufferMax to work. If that's the case, we should probably file a bug. Make it minor since I don't immediately see derby.storage.sortBufferMax documented. Also if you are setting it in your test to work around the issue, check your test and see if it works for an insane build. Typically the derby.debug.* code is not used for insane builds.
        Hide
        Tiago R. Espinha added a comment -

        One more update: it seems like I fixed it. I set another property on that file (that is actually set on the original test):
        derby.debug.true=testSort

        and I do get an external sort now.

        Weird thing that we need the debug enabled to get external sorts to happen, but well, it's fixed now and for this test that's what matters

        Thank you for your help.

        Show
        Tiago R. Espinha added a comment - One more update: it seems like I fixed it. I set another property on that file (that is actually set on the original test): derby.debug.true=testSort and I do get an external sort now. Weird thing that we need the debug enabled to get external sorts to happen, but well, it's fixed now and for this test that's what matters Thank you for your help.
        Hide
        Tiago R. Espinha added a comment -

        That's an improvement at least now the property does get set as we can see from the output:
        -------------------8<--------------------------
        2009-03-28 16:04:06.418 GMT : Security manager installed using the Basic server security policy.
        2009-03-28 16:04:07.063 GMT : Apache Derby Network Server - 10.6.0.0 alpha - (757793M) started and ready to accept connections on port 1527
        Sorting is booting and we have: 5
        Sorting and defaultSortBufferMax is 5
        --------------------8<-------------------------

        The crux of it is that the statistics data still reports as doing an internal sort:
        -------------------8<------------------------------------------------------------------
        Source result set:
        Sort ResultSet:
        Number of opens = 1
        Rows input = 416
        Rows returned = 416
        Eliminate duplicates = false
        In sorted order = false
        Sort information:
        Number of rows input=416
        Number of rows output=416
        Sort type=internal
        constructor time (milliseconds) = 0
        open time (milliseconds) = 0
        next time (milliseconds) = 0
        close time (milliseconds) = 0
        optimizer estimated row count: 422.00
        optimizer estimated cost: 140.25
        -------------------8<------------------------------------------------------------------
        (The table is slightly smaller now, just 416 records, but that shouldn't be an issue with a sortBufferMax of 5)

        So, I think I'm looking at two different issues here:

        • This specific property can't be set using SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY();
          Or in other words, it can be set but then Derby won't find it. This will probably not be an issue for my actual test, since I do it programmatically there. Maybe this is even normal and it was just a mistake of mine all along when testing manually.
        • The other, actual issue, is that Derby still refuses to do external sorting, even when the record count clearly exceeds the sortBufferMax. This one still remains a mystery.
        Show
        Tiago R. Espinha added a comment - That's an improvement at least now the property does get set as we can see from the output: ------------------- 8< -------------------------- 2009-03-28 16:04:06.418 GMT : Security manager installed using the Basic server security policy. 2009-03-28 16:04:07.063 GMT : Apache Derby Network Server - 10.6.0.0 alpha - (757793M) started and ready to accept connections on port 1527 Sorting is booting and we have: 5 Sorting and defaultSortBufferMax is 5 -------------------- 8< ------------------------- The crux of it is that the statistics data still reports as doing an internal sort: ------------------- 8< ------------------------------------------------------------------ Source result set: Sort ResultSet: Number of opens = 1 Rows input = 416 Rows returned = 416 Eliminate duplicates = false In sorted order = false Sort information: Number of rows input=416 Number of rows output=416 Sort type=internal constructor time (milliseconds) = 0 open time (milliseconds) = 0 next time (milliseconds) = 0 close time (milliseconds) = 0 optimizer estimated row count: 422.00 optimizer estimated cost: 140.25 ------------------- 8< ------------------------------------------------------------------ (The table is slightly smaller now, just 416 records, but that shouldn't be an issue with a sortBufferMax of 5) So, I think I'm looking at two different issues here: This specific property can't be set using SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY(); Or in other words, it can be set but then Derby won't find it. This will probably not be an issue for my actual test, since I do it programmatically there. Maybe this is even normal and it was just a mistake of mine all along when testing manually. The other, actual issue, is that Derby still refuses to do external sorting, even when the record count clearly exceeds the sortBufferMax. This one still remains a mystery.
        Hide
        Bryan Pendleton added a comment -

        What happens if you set the property using the techniques described at:
        http://db.apache.org/derby/docs/10.4/tuning/ctunsetprop16827.html

        This (older) page may also be helpful:
        http://db.apache.org/derby/manuals/tuning/perf14.html

        Show
        Bryan Pendleton added a comment - What happens if you set the property using the techniques described at: http://db.apache.org/derby/docs/10.4/tuning/ctunsetprop16827.html This (older) page may also be helpful: http://db.apache.org/derby/manuals/tuning/perf14.html
        Hide
        Tiago R. Espinha added a comment -

        I get that output indeed, but only once I do a SELECT that involves sorting.

        I'm doing this test manually now, I'm running the Network Server manually from my jars/sane/ and then throwing queries at it from ij.

        I also found this pretty old Cloudscape documentation: http://www.dwfa.ca/Library/Java/Cloudscape/v3.6.1/doc/html/coredocs/proper39.htm and it does say there "This property is static; if you change it while Cloudscape is running, the change does not take effect until you reboot."

        I did try this. I tried setting the value with the
        SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY('derby.storage.sortBufferMax','5');
        and then restarted the server. What I found is that the value is kept between "sessions" but ExternalSortFactory still does not seem to see it.

        In a desperate attempt, I also ignored the warning in service.properties and set the derby.storage.sortBufferMax there manually to 5, also with no luck.

        Show
        Tiago R. Espinha added a comment - I get that output indeed, but only once I do a SELECT that involves sorting. I'm doing this test manually now, I'm running the Network Server manually from my jars/sane/ and then throwing queries at it from ij. I also found this pretty old Cloudscape documentation: http://www.dwfa.ca/Library/Java/Cloudscape/v3.6.1/doc/html/coredocs/proper39.htm and it does say there "This property is static; if you change it while Cloudscape is running, the change does not take effect until you reboot." I did try this. I tried setting the value with the SYSCS_UTIL.SYSCS_SET_DATABASE_PROPERTY('derby.storage.sortBufferMax','5'); and then restarted the server. What I found is that the value is kept between "sessions" but ExternalSortFactory still does not seem to see it. In a desperate attempt, I also ignored the warning in service.properties and set the derby.storage.sortBufferMax there manually to 5, also with no luck.
        Hide
        Bryan Pendleton added a comment -

        So in your test, you see the output "Sorting is booting and we have: 0"?

        I wonder if the issue is that Derby is initializing itself and checking for this
        property before your test has had a chance to set the value.

        I think that our test harness has several different variations of ways to
        set system properties, and perhaps we need to use a different testing API
        to set the property so that the boot() method will see the value that we intend.

        Show
        Bryan Pendleton added a comment - So in your test, you see the output "Sorting is booting and we have: 0"? I wonder if the issue is that Derby is initializing itself and checking for this property before your test has had a chance to set the value. I think that our test harness has several different variations of ways to set system properties, and perhaps we need to use a different testing API to set the property so that the boot() method will see the value that we intend.
        Hide
        Tiago R. Espinha added a comment -

        Wow, you're right on the money.

        I added the following line right after that else:
        System.out.print("Sorting and defaultSortBufferMax is " + sortBufferMax);

        And also added the line:
        System.out.println("Sorting is booting and we have: "+defaultSortBufferMax);

        to the boot method, right after:
        defaultSortBufferMax = PropertyUtil.getSystemInt("derby.storage.sortBufferMax",
        0, Integer.MAX_VALUE, 0);

        The output is as follows:
        2009-03-28 14:54:10.432 GMT : Apache Derby Network Server - 10.6.0.0 alpha - (757793M) started and ready to accept connections on port 1527
        Sorting is booting and we have: 0
        Sorting and defaultSortBufferMax is 6168

        Also, I ran this on ij:
        values SYSCS_UTIL.SYSCS_GET_DATABASE_PROPERTY('derby.storage.sortBufferMax');

        and the output is 5 (like I set it to be).

        Could this be a bug or are we missing something?

        Show
        Tiago R. Espinha added a comment - Wow, you're right on the money. I added the following line right after that else: System.out.print("Sorting and defaultSortBufferMax is " + sortBufferMax); And also added the line: System.out.println("Sorting is booting and we have: "+defaultSortBufferMax); to the boot method, right after: defaultSortBufferMax = PropertyUtil.getSystemInt("derby.storage.sortBufferMax", 0, Integer.MAX_VALUE, 0); The output is as follows: 2009-03-28 14:54:10.432 GMT : Apache Derby Network Server - 10.6.0.0 alpha - (757793M) started and ready to accept connections on port 1527 Sorting is booting and we have: 0 Sorting and defaultSortBufferMax is 6168 Also, I ran this on ij: values SYSCS_UTIL.SYSCS_GET_DATABASE_PROPERTY('derby.storage.sortBufferMax'); and the output is 5 (like I set it to be). Could this be a bug or are we missing something?
        Hide
        Bryan Pendleton added a comment -

        I agree. I think we wanted to see sort type 'external' here.

        It looks like the code that handles derby.storage.sortBufferMax is around
        line 210 of org.apache.derby.impl.store.access.sort.ExternalSortFactory:

        else

        { // if user specified derby.storage.sortBufferMax, use it. sortBufferMax = defaultSortBufferMax; }

        Maybe you can investigate whether this code is actually seeing the value
        of sortBufferMax = 5?

        Show
        Bryan Pendleton added a comment - I agree. I think we wanted to see sort type 'external' here. It looks like the code that handles derby.storage.sortBufferMax is around line 210 of org.apache.derby.impl.store.access.sort.ExternalSortFactory: else { // if user specified derby.storage.sortBufferMax, use it. sortBufferMax = defaultSortBufferMax; } Maybe you can investigate whether this code is actually seeing the value of sortBufferMax = 5?
        Hide
        Tiago R. Espinha added a comment -

        Here's what I get from the statistics after having:
        1) Set sortBufferMax to 5
        2) Made a Select statement with Order By to 1 column, on a table with roughly 6000 records of assorted data.

        Source result set:
        Sort ResultSet:
        Number of opens = 1
        Rows input = 6148
        Rows returned = 6148
        Eliminate duplicates = false
        In sorted order = false
        Sort information:
        Number of rows input=6148
        Number of rows output=6148
        Sort type=internal
        constructor time (milliseconds) = 0
        open time (milliseconds) = 0
        next time (milliseconds) = 0
        close time (milliseconds) = 0
        optimizer estimated row count: 5232.00
        optimizer estimated cost: 1105.97

        Now, sort type=internal? I would have thought that it would give me "external" since the sortBufferMax is set to 5 and we have 6000 rows to sort, but nope. What can I possibly be doing wrong?

        Show
        Tiago R. Espinha added a comment - Here's what I get from the statistics after having: 1) Set sortBufferMax to 5 2) Made a Select statement with Order By to 1 column, on a table with roughly 6000 records of assorted data. Source result set: Sort ResultSet: Number of opens = 1 Rows input = 6148 Rows returned = 6148 Eliminate duplicates = false In sorted order = false Sort information: Number of rows input=6148 Number of rows output=6148 Sort type=internal constructor time (milliseconds) = 0 open time (milliseconds) = 0 next time (milliseconds) = 0 close time (milliseconds) = 0 optimizer estimated row count: 5232.00 optimizer estimated cost: 1105.97 Now, sort type=internal? I would have thought that it would give me "external" since the sortBufferMax is set to 5 and we have 6000 rows to sort, but nope. What can I possibly be doing wrong?
        Hide
        Kathey Marsden added a comment -

        There is the less than perfect RuntimeStatisticsParser which could potentially be enhanced to determine this. You can look at PredicatePushdownTest for an example of usage.

        Show
        Kathey Marsden added a comment - There is the less than perfect RuntimeStatisticsParser which could potentially be enhanced to determine this. You can look at PredicatePushdownTest for an example of usage.
        Hide
        Tiago R. Espinha added a comment -

        Ah, this actually makes sense to me now

        I'll look into finding those runtime statistics as I think this could indeed make the test more trustworthy for its purpose. If due to some bug, this overflow doesn't happen and all the data is kept in memory, then the results produced by this test can't be trusted.

        Show
        Tiago R. Espinha added a comment - Ah, this actually makes sense to me now I'll look into finding those runtime statistics as I think this could indeed make the test more trustworthy for its purpose. If due to some bug, this overflow doesn't happen and all the data is kept in memory, then the results produced by this test can't be trusted.
        Hide
        Bryan Pendleton added a comment -

        I don't think the precise location of the commit or of the sort buffer size matters, because the sorting occurs before
        the first record has been retrieved, so as long as the commit is after the first record has
        been retrieved, whether the overflow-to-disk occurred on the 4th or 5th record doesn't really matter.

        What matters is that the commit doesn't close and release the temporary sorted records; that
        would cause some sort of internal error fetching records after the commit if it occurred.

        Ideally, the test would verify that the sort actually did overflow to disk; I think there is some
        information in the runtime statistics that will prove this. You could also verify that external
        sorting occurred by watching the sorter's behavior in the debugger, I suppose.

        Show
        Bryan Pendleton added a comment - I don't think the precise location of the commit or of the sort buffer size matters, because the sorting occurs before the first record has been retrieved, so as long as the commit is after the first record has been retrieved, whether the overflow-to-disk occurred on the 4th or 5th record doesn't really matter. What matters is that the commit doesn't close and release the temporary sorted records; that would cause some sort of internal error fetching records after the commit if it occurred. Ideally, the test would verify that the sort actually did overflow to disk; I think there is some information in the runtime statistics that will prove this. You could also verify that external sorting occurred by watching the sorter's behavior in the debugger, I suppose.
        Hide
        Tiago R. Espinha added a comment - - edited

        I have started fixing the problems in my patch but you noticed one thing that had totally skipped me: the comments mention that the cutover has been set to 4 rows, but even in the old test it was already set to 5 rows.

        This is wrong, right?

        I'm just trying to make heads or tails of whether this has a direct implication on when we do the commits on the fixtures.

        I might just be about to say the wrongest thing ever but bear with me: having the sortBufferMax set to 5, that means that when we're doing a sort operation that involves more than 5 records, Derby will automatically use the hard drive to do the sort.

        What are the implications of this from a test perspective? Do we need to commit() before or after the 5th record is pulled?

        This is just a little confusing considering that the comments say one thing and the actual parameters tell a different story. It leaves me wondering whether the old test had already been adapted for a sortBufferMax of 5 or if instead, it was still thought for a value of 4.

        Some thoughts before I issue another patch please?

        Show
        Tiago R. Espinha added a comment - - edited I have started fixing the problems in my patch but you noticed one thing that had totally skipped me: the comments mention that the cutover has been set to 4 rows, but even in the old test it was already set to 5 rows. This is wrong, right? I'm just trying to make heads or tails of whether this has a direct implication on when we do the commits on the fixtures. I might just be about to say the wrongest thing ever but bear with me: having the sortBufferMax set to 5, that means that when we're doing a sort operation that involves more than 5 records, Derby will automatically use the hard drive to do the sort. What are the implications of this from a test perspective? Do we need to commit() before or after the 5th record is pulled? This is just a little confusing considering that the comments say one thing and the actual parameters tell a different story. It leaves me wondering whether the old test had already been adapted for a sortBufferMax of 5 or if instead, it was still thought for a value of 4. Some thoughts before I issue another patch please?
        Hide
        Kathey Marsden added a comment -

        Thank you Tiago for picking up this issue. I know it can be tricky picking up a patch that someone else started. Here are my comments:

        There are methods in BaseJDBCTestCase that can be used instead of calling getConnection().xxx

        Instead of getConnection().setAutoCommit(false), you can just call setAutoComit(false0

        Instead of getConnection().createStatement() and getConnection.prepareStatement() you can just call createStatement() or prepareStatment and the statements will get closed automatically so you can take out the closing of the statements. We may not have helper methods for the cases where you need to specify extra arguments. In those cases, what you have is fine.

        And also just commit() instead of getConnection.commit();

        Let's make a separate table for the test with the added rows.

        There are properties in the derby properties for the old test that are not getting set in the new one:

        derby.storage.sortBufferMax=5
        derby.debug.true=testSort

        There is a SystemPropertiesTestSetup class that you can use for setting the properties.

        testOrder_Hold()

        Comment in test says: "Cutover to external sort has been set to 4 rows by the test property file", but should reflect that we are actually now setting it by Sytem property and that it is actually 5. Similar comments are in the other fixtures.

        I see 10 elements in boolean[] doCommitAfter =

        {true, false, false, false, true, false, false, false, true, true}

        ;

        If I am reading the master, there seems to be some discrepancy between the master and the number and sequence of next calls and commits and the doCommitAfter array /for loop in the new test. Could you please recount carefully and fix this up. I don't fully understand the store code paths being tested but I think that they are farily specific with this order.

        testOrder_NoHold()
        again the for loop seems to be missing a row and we don't do/check the final rs.next() which is false.

        testOrderWthMultipleLevel()
        On this one I didn't carefully double check the commit math. Again, please take a close look before you submit the next patch.

        Show
        Kathey Marsden added a comment - Thank you Tiago for picking up this issue. I know it can be tricky picking up a patch that someone else started. Here are my comments: There are methods in BaseJDBCTestCase that can be used instead of calling getConnection().xxx Instead of getConnection().setAutoCommit(false), you can just call setAutoComit(false0 Instead of getConnection().createStatement() and getConnection.prepareStatement() you can just call createStatement() or prepareStatment and the statements will get closed automatically so you can take out the closing of the statements. We may not have helper methods for the cases where you need to specify extra arguments. In those cases, what you have is fine. And also just commit() instead of getConnection.commit(); Let's make a separate table for the test with the added rows. There are properties in the derby properties for the old test that are not getting set in the new one: derby.storage.sortBufferMax=5 derby.debug.true=testSort There is a SystemPropertiesTestSetup class that you can use for setting the properties. testOrder_Hold() Comment in test says: "Cutover to external sort has been set to 4 rows by the test property file", but should reflect that we are actually now setting it by Sytem property and that it is actually 5. Similar comments are in the other fixtures. I see 10 elements in boolean[] doCommitAfter = {true, false, false, false, true, false, false, false, true, true} ; If I am reading the master, there seems to be some discrepancy between the master and the number and sequence of next calls and commits and the doCommitAfter array /for loop in the new test. Could you please recount carefully and fix this up. I don't fully understand the store code paths being tested but I think that they are farily specific with this order. testOrder_NoHold() again the for loop seems to be missing a row and we don't do/check the final rs.next() which is false. testOrderWthMultipleLevel() On this one I didn't carefully double check the commit math. Again, please take a close look before you submit the next patch.
        Hide
        Tiago R. Espinha added a comment -

        The tests just finished and both suites.All and derbyall ran with no errors.

        Show
        Tiago R. Espinha added a comment - The tests just finished and both suites.All and derbyall ran with no errors.
        Hide
        Tiago R. Espinha added a comment -

        I know I shouldn't delete old patches, it's just that the first one I uploaded was missing the 'svn add' of the actual test file, so it was completely unusable. The only difference between them is just that.

        Show
        Tiago R. Espinha added a comment - I know I shouldn't delete old patches, it's just that the first one I uploaded was missing the 'svn add' of the actual test file, so it was completely unusable. The only difference between them is just that.
        Hide
        Kathey Marsden added a comment -

        Hi Tiago,

        Thanks for the patches.Please don't delete previous patches, even if they have problems. It makes it hard to make sense of the comment history. You can either give the new patch the same name and Jira will nicely gray out the old ones or you can give it a different name e.g. derby-3842-tiago2.patch, whichever is your preference.

        Show
        Kathey Marsden added a comment - Hi Tiago, Thanks for the patches.Please don't delete previous patches, even if they have problems. It makes it hard to make sense of the comment history. You can either give the new patch the same name and Jira will nicely gray out the old ones or you can give it a different name e.g. derby-3842-tiago2.patch, whichever is your preference.
        Hide
        Tiago R. Espinha added a comment -

        Fixed patch. Previous one had an issue.

        Show
        Tiago R. Espinha added a comment - Fixed patch. Previous one had an issue.
        Hide
        Tiago R. Espinha added a comment -

        I attached a patch although I believe it will not be the final one yet.

        A few corrections to myself after having talked with Kathey:

        • This patch makes use of a single table, populated on decorateSQL. This might not be a good plan in the end, as one of the tests adds data to the table. Then again, the data that is added does not interfere with the data that the other two tests need - so is it wise to leave it as it is?

        All in all, it is order-independent even if one of the tests INSERTs into the table.

        • This patch is now looking at both the value of "a" and the actual data, instead of the row number. I had misunderstood Mamta. What was being done before was that we were looking at the cursor's row number. I switched this behaviour to look at the data in the table instead, like the original test did.

        Finally, this patch is largely based on Junjie's work (lots of copy-paste and adaptations), so it is be possible that I have missed something.

        Any thoughts or comments are appreciated. I'll post here the results of the test running once I have them.

        Show
        Tiago R. Espinha added a comment - I attached a patch although I believe it will not be the final one yet. A few corrections to myself after having talked with Kathey: This patch makes use of a single table, populated on decorateSQL. This might not be a good plan in the end, as one of the tests adds data to the table. Then again, the data that is added does not interfere with the data that the other two tests need - so is it wise to leave it as it is? All in all, it is order-independent even if one of the tests INSERTs into the table. This patch is now looking at both the value of "a" and the actual data, instead of the row number. I had misunderstood Mamta. What was being done before was that we were looking at the cursor's row number. I switched this behaviour to look at the data in the table instead, like the original test did. Finally, this patch is largely based on Junjie's work (lots of copy-paste and adaptations), so it is be possible that I have missed something. Any thoughts or comments are appreciated. I'll post here the results of the test running once I have them.
        Hide
        Tiago R. Espinha added a comment -

        I have done some modifications to this test, but now on a second read of the comments I'm not so sure they were good changes.

        I see we have a setUp and a tearDown, but for JDBC tests can't/shouldn't we just use the decorateSQL to create and populate the table?

        Additionally, I see that Mamta suggested that we drop the foo table after the fixture testOrderWithMultipleLevel. On the actual patch by Junjie the table is being dropped at the end of each fixture but I'm not sure about this.

        On the original test it seems to me that the table is created and populated once and then it is queried on several tests, having only the function, procedure and the table dropped at the end of the whole set.

        Can't this influence the outcome and the purpose of this test? And maybe even make it slower unnecessarily?

        Finally, I will make it so that we also look at the actual data, since that is actually done on the original test. Although the relevance of this is still debatable I believe. If the cursor isn't «held», then the row number will also be different than expected since this isn't quite a row number but just a regular numeric key that is initially inserted in the database.

        Show
        Tiago R. Espinha added a comment - I have done some modifications to this test, but now on a second read of the comments I'm not so sure they were good changes. I see we have a setUp and a tearDown, but for JDBC tests can't/shouldn't we just use the decorateSQL to create and populate the table? Additionally, I see that Mamta suggested that we drop the foo table after the fixture testOrderWithMultipleLevel. On the actual patch by Junjie the table is being dropped at the end of each fixture but I'm not sure about this. On the original test it seems to me that the table is created and populated once and then it is queried on several tests, having only the function, procedure and the table dropped at the end of the whole set. Can't this influence the outcome and the purpose of this test? And maybe even make it slower unnecessarily? Finally, I will make it so that we also look at the actual data, since that is actually done on the original test. Although the relevance of this is still debatable I believe. If the cursor isn't «held», then the row number will also be different than expected since this isn't quite a row number but just a regular numeric key that is initially inserted in the database.
        Hide
        Knut Anders Hatlen added a comment -

        From the comments it seems like the patch is not ready for commit, so I'm unchecking Patch Available.

        Show
        Knut Anders Hatlen added a comment - From the comments it seems like the patch is not ready for commit, so I'm unchecking Patch Available.
        Hide
        Mamta A. Satoor added a comment -

        Thanks for working on the test conversion. It looks pretty good. Just couple more comments.
        1)I think it would be good to drop the table foo at the end of fixture testOrderWthMultipleLevel
        2)Also, like with the other hold cursor tests, I am not sure if it is enough to check the row number after doing a "next" or if we should actually be looking at the actual data returned.

        Show
        Mamta A. Satoor added a comment - Thanks for working on the test conversion. It looks pretty good. Just couple more comments. 1)I think it would be good to drop the table foo at the end of fixture testOrderWthMultipleLevel 2)Also, like with the other hold cursor tests, I am not sure if it is enough to check the row number after doing a "next" or if we should actually be looking at the actual data returned.
        Hide
        Mamta A. Satoor added a comment -

        The .sql test uses function PADSTRING to insert data as shown below
        insert into foo values
        (10,PADSTRING('10',2000)), (9,PADSTRING('9',2000)), (8,PADSTRING('8',2000)), (7,PADSTRING('7',2000)), (6,PADSTRING('6',2000)), (5,PADSTRING('5',2000)), (4,PADSTRING('4',2000)), (3,PADSTRING('3',2000)), (2,PADSTRING('2',2000)), (1,PADSTRING('1',2000));

        The junit test does following to replace the usage of PADSTRING in .sql test
        + String[] rows = new String[10];
        + for(int i = rows.length; i >= 1; i--)

        { + rows[i - 1] = Formatters.padString("" + i, 2000); + }

        +
        + Statement stUtil = createStatement();
        + stUtil.executeUpdate("create table foo (a int, data varchar(2000))");
        + stUtil.close();
        +
        + PreparedStatement ps = prepareStatement(
        + "insert into foo values(?,?), (?,?), (?,?), (?,?), (?,?), " +
        + "(?,?), (?,?), (?,?), (?,?), (?,?)");
        + for(int i = 0; i < rows.length; i++)

        { + ps.setInt(i * 2 + 1, rows.length - i); + ps.setString(i * 2 + 2, rows[rows.length - i - 1]); + }

        + ps.executeUpdate();
        + ps.close();

        I think to imitate the original test, the junit test should create a PADSTRING function and use that to load data into the table. The example of that can be found in StressMultiTest

        Show
        Mamta A. Satoor added a comment - The .sql test uses function PADSTRING to insert data as shown below insert into foo values (10,PADSTRING('10',2000)), (9,PADSTRING('9',2000)), (8,PADSTRING('8',2000)), (7,PADSTRING('7',2000)), (6,PADSTRING('6',2000)), (5,PADSTRING('5',2000)), (4,PADSTRING('4',2000)), (3,PADSTRING('3',2000)), (2,PADSTRING('2',2000)), (1,PADSTRING('1',2000)); The junit test does following to replace the usage of PADSTRING in .sql test + String[] rows = new String [10] ; + for(int i = rows.length; i >= 1; i--) { + rows[i - 1] = Formatters.padString("" + i, 2000); + } + + Statement stUtil = createStatement(); + stUtil.executeUpdate("create table foo (a int, data varchar(2000))"); + stUtil.close(); + + PreparedStatement ps = prepareStatement( + "insert into foo values(?,?), (?,?), (?,?), (?,?), (?,?), " + + "(?,?), (?,?), (?,?), (?,?), (?,?)"); + for(int i = 0; i < rows.length; i++) { + ps.setInt(i * 2 + 1, rows.length - i); + ps.setString(i * 2 + 2, rows[rows.length - i - 1]); + } + ps.executeUpdate(); + ps.close(); I think to imitate the original test, the junit test should create a PADSTRING function and use that to load data into the table. The example of that can be found in StressMultiTest
        Hide
        Mamta A. Satoor added a comment -

        I am looking at the patch but I am curious, don't we need to add the converted test to some _Suite.java?

        Show
        Mamta A. Satoor added a comment - I am looking at the patch but I am curious, don't we need to add the converted test to some _Suite.java?
        Hide
        Junjie Peng added a comment -

        Please check the patch, thanks!

        Show
        Junjie Peng added a comment - Please check the patch, thanks!

          People

          • Assignee:
            Tiago R. Espinha
            Reporter:
            Junjie Peng
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development