Derby
  1. Derby
  2. DERBY-3173

Removed cached String objects from SQLDate, SQLTime and SQLTimestamp

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.7.1.1
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      Newcomer

      Description

      These type classes save a copy of the value when it is converted to a String (e.g. through a ResultSet.getString()). This complicates the code & increases memory use for little value, in most cases the cached value will never be used. E.g. for any type of scan the String value will be discarded when moving to the next row. In most cases applications do not call getString() twice on a column.

      The code has some historical basis in the fact that these types used to be represented by a java.sql.Time/Date/Timestamp object and its conversion to String was slow. Now the conversion of all these types to a String is simple.

      In addition I think the getString() will sometimes return a non-normalized form, if the value is set by a non-standard format then the cached String is set to the non-standard format, not the standard format, I believe this is incorrect.

      1. Derby-3173.diff
        2 kB
        Eranda Sooriyabandara
      2. sqldate.diff
        2 kB
        Knut Anders Hatlen
      3. Derby-3173.diff
        5 kB
        Eranda Sooriyabandara
      4. Derby-3173.diff
        6 kB
        Eranda Sooriyabandara
      5. d3173.diff
        7 kB
        Knut Anders Hatlen
      6. d3173_warning.diff
        0.6 kB
        Eranda Sooriyabandara

        Activity

        Hide
        Kathey Marsden added a comment -

        Sounds appropriate for a newcomer. Please correct if I am wrong.

        Show
        Kathey Marsden added a comment - Sounds appropriate for a newcomer. Please correct if I am wrong.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi,
        Where is this caching done? Also appreciate if can you provide more details which will be helpful when proceed with this issue.
        Thanks

        Show
        Eranda Sooriyabandara added a comment - Hi, Where is this caching done? Also appreciate if can you provide more details which will be helpful when proceed with this issue. Thanks
        Hide
        Knut Anders Hatlen added a comment -

        Hi Eranda,
        Take a look at the field named valueString in the classes SQLDate, SQLTime and SQLTimestamp. I think what Dan suggested was that this field was removed, and getString() was changed so that it always regenerates the string representation of the object.

        Show
        Knut Anders Hatlen added a comment - Hi Eranda, Take a look at the field named valueString in the classes SQLDate, SQLTime and SQLTimestamp. I think what Dan suggested was that this field was removed, and getString() was changed so that it always regenerates the string representation of the object.
        Hide
        Knut Anders Hatlen added a comment -

        Dan wrote:

        > In addition I think the getString() will sometimes return a
        > non-normalized form, if the value is set by a non-standard format
        > then the cached String is set to the non-standard format, not the
        > standard format, I believe this is incorrect.

        This part of the issue sounds like the problem that caused DERBY-3856,
        which has later been fixed.

        Show
        Knut Anders Hatlen added a comment - Dan wrote: > In addition I think the getString() will sometimes return a > non-normalized form, if the value is set by a non-standard format > then the cached String is set to the non-standard format, not the > standard format, I believe this is incorrect. This part of the issue sounds like the problem that caused DERBY-3856 , which has later been fixed.
        Hide
        Eranda Sooriyabandara added a comment - - edited

        Hi Knut,
        According to your guidance I came up with a patch to remove caching the value of SQLDate.
        Here I remove the part "ClassSize.estimateMemoryUsage( valueString)" which allocate the memory for caching. And declare the valueString inside the getString.
        Is it what addressed here?
        thanks

        Show
        Eranda Sooriyabandara added a comment - - edited Hi Knut, According to your guidance I came up with a patch to remove caching the value of SQLDate. Here I remove the part "ClassSize.estimateMemoryUsage( valueString)" which allocate the memory for caching. And declare the valueString inside the getString. Is it what addressed here? thanks
        Hide
        Knut Anders Hatlen added a comment -

        Hi Eranda,

        Thanks for posting the patch. Your changes look good to me. I'm running the regression tests now and intend to commit it if the tests run cleanly. Before starting the tests, I made two minor changes to your patch (see the attached sqldate.diff): 1) Removed trailing blanks. 2) Narrowed the scope of the local variable valueString in getString() since it's only used inside the first branch of the if statement.

        Show
        Knut Anders Hatlen added a comment - Hi Eranda, Thanks for posting the patch. Your changes look good to me. I'm running the regression tests now and intend to commit it if the tests run cleanly. Before starting the tests, I made two minor changes to your patch (see the attached sqldate.diff): 1) Removed trailing blanks. 2) Narrowed the scope of the local variable valueString in getString() since it's only used inside the first branch of the if statement.
        Hide
        Knut Anders Hatlen added a comment -

        All tests ran cleanly. Committed revision 988107.

        I'm leaving the issue open until SQLTime and SQLTimestamp have been fixed.

        Show
        Knut Anders Hatlen added a comment - All tests ran cleanly. Committed revision 988107. I'm leaving the issue open until SQLTime and SQLTimestamp have been fixed.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Knut,
        I do some updates in SQLTime and SQLTimestamp as I did in SQLDate and run the regression tests which ran successfully. Here I am attaching the patch for your concern.
        thanks

        Show
        Eranda Sooriyabandara added a comment - Hi Knut, I do some updates in SQLTime and SQLTimestamp as I did in SQLDate and run the regression tests which ran successfully. Here I am attaching the patch for your concern. thanks
        Hide
        Knut Anders Hatlen added a comment -

        Thank you, Eranda. The patch looks fine to me, but I think there's a small problem with this change in SQLTime:

        @@ -551,7 +526,6 @@
        else if( hour > 12)
        throw StandardException.newException( SQLState.LANG_DATE_RANGE_EXCEPTION);
        }

        • valueString = parser.checkEnd();
          encodedTime = computeEncodedTime( hour, minute, second);
          }
          else

        The call to checkEnd() performs some extra input validation that we shouldn't remove. For example, the following query used to be rejected:

        ij> values time('05 am pm');
        ERROR 22007: The syntax of the string representation of a datetime value is incorrect.

        With the patch, the query is now accepted:

        ij> values time('05 am pm');
        1
        --------
        05:00:00

        1 row selected

        Instead of removing that line completely, I think we should keep the call to checkEnd() and just discard the return value.

        Show
        Knut Anders Hatlen added a comment - Thank you, Eranda. The patch looks fine to me, but I think there's a small problem with this change in SQLTime: @@ -551,7 +526,6 @@ else if( hour > 12) throw StandardException.newException( SQLState.LANG_DATE_RANGE_EXCEPTION); } valueString = parser.checkEnd(); encodedTime = computeEncodedTime( hour, minute, second); } else The call to checkEnd() performs some extra input validation that we shouldn't remove. For example, the following query used to be rejected: ij> values time('05 am pm'); ERROR 22007: The syntax of the string representation of a datetime value is incorrect. With the patch, the query is now accepted: ij> values time('05 am pm'); 1 -------- 05:00:00 1 row selected Instead of removing that line completely, I think we should keep the call to checkEnd() and just discard the return value.
        Hide
        Knut Anders Hatlen added a comment -

        The latest patch opens up for some simplifications in the DateTimeParser class too:

        • the getTrimmedString() method isn't called anymore, so it can be removed
        • none of the callers use the return value from checkEnd() anymore, so the return type could be changed to void
        • the field trimmedString can be removed
        Show
        Knut Anders Hatlen added a comment - The latest patch opens up for some simplifications in the DateTimeParser class too: the getTrimmedString() method isn't called anymore, so it can be removed none of the callers use the return value from checkEnd() anymore, so the return type could be changed to void the field trimmedString can be removed
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Knut,
        Thanks for got the thing and I did the changes as you suggest and here I am attaching the patch with this. It seems to be working now.

        ij> values time('05 am pm');
        ERROR 22007: The syntax of the string representation of a datetime value is incorrect.
        ij> values time('05 am');
        1
        --------
        05:00:00

        1 row selected
        ij> values time('05 pm');
        1
        --------
        17:00:00

        1 row selected

        thanks

        Show
        Eranda Sooriyabandara added a comment - Hi Knut, Thanks for got the thing and I did the changes as you suggest and here I am attaching the patch with this. It seems to be working now. ij> values time('05 am pm'); ERROR 22007: The syntax of the string representation of a datetime value is incorrect. ij> values time('05 am'); 1 -------- 05:00:00 1 row selected ij> values time('05 pm'); 1 -------- 17:00:00 1 row selected thanks
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Eranda. I made two small changes to the patch before I committed it:

        1) Adjusted the indentation a couple of places. (It looks like your IDE is configured with tab size 8, whereas the code is written with tab size 4.)

        2) Removed more code from the checkEnd() method (there was a loop that only modified the local variable end, which was no longer in use)

        Committed revision 989918.

        Show
        Knut Anders Hatlen added a comment - Thanks, Eranda. I made two small changes to the patch before I committed it: 1) Adjusted the indentation a couple of places. (It looks like your IDE is configured with tab size 8, whereas the code is written with tab size 4.) 2) Removed more code from the checkEnd() method (there was a loop that only modified the local variable end, which was no longer in use) Committed revision 989918.
        Hide
        Eranda Sooriyabandara added a comment -

        Thanks Knut for committing. I am closing this issue.

        Show
        Eranda Sooriyabandara added a comment - Thanks Knut for committing. I am closing this issue.
        Hide
        Eranda Sooriyabandara added a comment -

        Due to javadoc warning which appear due to comment error.
        [javadoc] C:\trunk\java\engine\org\apache\derby\iapi\types\DateTimeParser.java:191:
        warning - @return tag cannot be used in method with void return type.

        Show
        Eranda Sooriyabandara added a comment - Due to javadoc warning which appear due to comment error. [javadoc] C:\trunk\java\engine\org\apache\derby\iapi\types\DateTimeParser.java:191: warning - @return tag cannot be used in method with void return type.
        Hide
        Eranda Sooriyabandara added a comment -

        Here I am attaching the patch in which I remove the @return part.

        Show
        Eranda Sooriyabandara added a comment - Here I am attaching the patch in which I remove the @return part.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Eranda. Committed revision 991124.

        Show
        Knut Anders Hatlen added a comment - Thanks, Eranda. Committed revision 991124.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

          People

          • Assignee:
            Eranda Sooriyabandara
            Reporter:
            Daniel John Debrunner
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development