Derby
  1. Derby
  2. DERBY-5420

Regression suite appears locale sensitive: failed in TableLockBasicTest: bug in RealBasicNoPutResultSetStatistics

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.8.1.2
    • Fix Version/s: 10.9.1.0
    • Component/s: Localization, SQL, Test
    • Labels:
      None
    • Environment:
      Windows Vista SP2, Norwegian locale, JDK 7.
    • Bug behavior facts:
      Regression Test Failure

      Description

      TableLockBasicTest failed due to unexpected locale in the runtime statistics.

      The execution plans are asserted in this test and I saw this diff:

      Expected:
      :
      optimizer estimated row count: 6.00
      optimizer estimated cost: 100.40<
      Found:
      :
      optimizer estimated row count: 6,00
      optimizer estimated cost: 100,40<

      the latter using a decimal comma whereas a decimal point is expected.

      1. derby-5420-1.diff
        9 kB
        Dag H. Wanvik
      2. derby-5420-1.stat
        0.3 kB
        Dag H. Wanvik
      3. derby-5420-2.diff
        22 kB
        Dag H. Wanvik
      4. derby-5420-2.stat
        1 kB
        Dag H. Wanvik

        Activity

        Hide
        Dag H. Wanvik added a comment -

        I am a bit puzzled by this one, because it seems the test does set the default locale to Locale.ENGLISH (something which in my small test does give the expected decimal character..)

        Show
        Dag H. Wanvik added a comment - I am a bit puzzled by this one, because it seems the test does set the default locale to Locale.ENGLISH (something which in my small test does give the expected decimal character..)
        Hide
        Dag H. Wanvik added a comment -

        Running the test stand-alone I don't see an error, I'll rerun the suitesAll to see if that is of importance.

        Show
        Dag H. Wanvik added a comment - Running the test stand-alone I don't see an error, I'll rerun the suitesAll to see if that is of importance.
        Hide
        Dag H. Wanvik added a comment -

        This is now a confirmed Heisenbug.. I can only see it in a suite context, I have narrowed it down quite a bit, but it's proving elusive. I see it if I run only the derbynet suite ahead of the suite of the store test (in which this test resides). I have narrowed the store suite down to just running the two autoindex tests ahead of TableLockBasicTest, but it sometimes passes.

        Show
        Dag H. Wanvik added a comment - This is now a confirmed Heisenbug.. I can only see it in a suite context, I have narrowed it down quite a bit, but it's proving elusive. I see it if I run only the derbynet suite ahead of the suite of the store test (in which this test resides). I have narrowed the store suite down to just running the two autoindex tests ahead of TableLockBasicTest, but it sometimes passes.
        Hide
        Knut Anders Hatlen added a comment -

        Just a thought... Does it help if you change Locale.ENGLISH to Locale.US in the test setup? The former only specifies the language, whereas the latter specifies language and country. Maybe you can end up with some hybrid locale (like en_NO) when using the language-only variant. No idea why it would be intermittent if this is what's causing it, though.

        Show
        Knut Anders Hatlen added a comment - Just a thought... Does it help if you change Locale.ENGLISH to Locale.US in the test setup? The former only specifies the language, whereas the latter specifies language and country. Maybe you can end up with some hybrid locale (like en_NO) when using the language-only variant. No idea why it would be intermittent if this is what's causing it, though.
        Hide
        Dag H. Wanvik added a comment -

        Interesting idea, on my WIndows PC was actually running a "hybrid" locale: Norwegian language ("display language", but US style numbers & dates ("formatting"). Unfortunately, Locale.US didn't correct the error.

        Show
        Dag H. Wanvik added a comment - Interesting idea, on my WIndows PC was actually running a "hybrid" locale: Norwegian language ("display language", but US style numbers & dates ("formatting"). Unfortunately, Locale.US didn't correct the error.
        Hide
        Dag H. Wanvik added a comment - - edited

        It turns out that RealBasicNoPutResultSetStatistics#formatDouble caches the decimal format:
        :
        if (df == null)

        { // RESOLVE: This really should use the database locale to // format the number. df = new DecimalFormat("###########0.00"); df.setMinimumIntegerDigits(1); }

        and even though we currently have correct Locale (language is English and country US), the cached decimal format descriptor has the wrong decimal character, i.e. a comma, presumably from an earlier test which did not specify locale and thus inherited the default locale on my box. Since this is an engine static variable, it will remain in the wrong form until the engine is rebooted, hence the error. Now, I just need to figure out which test first gives it the wrong form As the comment indicates, this is broken...

        Show
        Dag H. Wanvik added a comment - - edited It turns out that RealBasicNoPutResultSetStatistics#formatDouble caches the decimal format: : if (df == null) { // RESOLVE: This really should use the database locale to // format the number. df = new DecimalFormat("###########0.00"); df.setMinimumIntegerDigits(1); } and even though we currently have correct Locale (language is English and country US), the cached decimal format descriptor has the wrong decimal character, i.e. a comma, presumably from an earlier test which did not specify locale and thus inherited the default locale on my box. Since this is an engine static variable, it will remain in the wrong form until the engine is rebooted, hence the error. Now, I just need to figure out which test first gives it the wrong form As the comment indicates, this is broken...
        Hide
        Dag H. Wanvik added a comment -

        A solution may be to save the default locale at the time of creating the decimal format, and check if the default locale has changed before deciding to use the hashed value or recompute it. The current code is an optimization, hopefully retrieving the current default locale will not void the optimization entirely.

        Show
        Dag H. Wanvik added a comment - A solution may be to save the default locale at the time of creating the decimal format, and check if the default locale has changed before deciding to use the hashed value or recompute it. The current code is an optimization, hopefully retrieving the current default locale will not void the optimization entirely.
        Hide
        Dag H. Wanvik added a comment -

        Just to clearify, the bug is not really intermittent after all, it just appeared that way, since depending on which tests ahead of TableLockBasicTest I commented out, the first setting of the decimal format above might happen in a test running with a specific English locale, or in a test running with the default one.

        For example, AccessTest calls SYSCS_GET_RUNTIMESTATISTICS without setting explicit locale, but this test uses the RuntimeStatisticsParser and doesn't worry about the double number estimated row count, so AccessTest test is not impacted by (an unexpected form of) decimal printing.

        Show
        Dag H. Wanvik added a comment - Just to clearify, the bug is not really intermittent after all, it just appeared that way, since depending on which tests ahead of TableLockBasicTest I commented out, the first setting of the decimal format above might happen in a test running with a specific English locale, or in a test running with the default one. For example, AccessTest calls SYSCS_GET_RUNTIMESTATISTICS without setting explicit locale, but this test uses the RuntimeStatisticsParser and doesn't worry about the double number estimated row count, so AccessTest test is not impacted by (an unexpected form of) decimal printing.
        Hide
        Bryan Pendleton added a comment -

        Great catch! And thanks for the clear description of the behavior; it makes complete sense.
        It's always nice to find a deterministic answer

        My initial reaction to the code you pasted in your earlier comment is that it seems like
        the sort of thing that should be done in the ResultSetStatistics constructor.

        That is, it seems like the "df" object should not be static, and the code to set it up based
        on the current locale should be in the constructor.

        I'm not sure if that would be a performance problem, but it might be a place to start.

        Show
        Bryan Pendleton added a comment - Great catch! And thanks for the clear description of the behavior; it makes complete sense. It's always nice to find a deterministic answer My initial reaction to the code you pasted in your earlier comment is that it seems like the sort of thing that should be done in the ResultSetStatistics constructor. That is, it seems like the "df" object should not be static, and the code to set it up based on the current locale should be in the constructor. I'm not sure if that would be a performance problem, but it might be a place to start.
        Hide
        Knut Anders Hatlen added a comment -

        I agree with Bryan that it sounds broken to cache the DecimalFormat in a static field. According to DecimalFormat's javadoc, it's not thread safe without external synchronization, so each result set needs to have its own instance.

        Another question is whether the correct fix is to use the default locale, or (as suggested by the comments in the code) the database locale. I lean towards the latter, as I think the rest of the runtime statistics output is generated with that locale.

        Show
        Knut Anders Hatlen added a comment - I agree with Bryan that it sounds broken to cache the DecimalFormat in a static field. According to DecimalFormat's javadoc, it's not thread safe without external synchronization, so each result set needs to have its own instance. Another question is whether the correct fix is to use the default locale, or (as suggested by the comments in the code) the database locale. I lean towards the latter, as I think the rest of the runtime statistics output is generated with that locale.
        Hide
        Knut Anders Hatlen added a comment -

        Just a thought... What if we change the message text of the RTS_OPT_EST_RC message in messages.xml from

        <text>optimizer estimated row count</text>
        to
        <text>optimizer estimated row count:

        {0,number,###########0.00}

        </text>

        and in RealBasicNoPutResultSetStatistics.dumpEstimatedCosts() from

        MessageService.getTextMessage(SQLState.RTS_OPT_EST_RC) +
        ": " +
        formatDouble(optimizerEstimatedRowCount) + "\n" +
        to
        MessageService.getTextMessage(SQLState.RTS_OPT_EST_RC, new Double(optimizerEstimatedRowCount)) + "\n" +
        ?

        Similar changes would be needed for RTS_OPT_EST_COST too.

        Then we avoid the need to explicitly create and cache a DecimalFormat instance, and the number will be formatted using the same locale as the rest of the message (the database locale). And we can remove the formatDouble() method, so we'll reduce the code size too.

        Show
        Knut Anders Hatlen added a comment - Just a thought... What if we change the message text of the RTS_OPT_EST_RC message in messages.xml from <text>optimizer estimated row count</text> to <text>optimizer estimated row count: {0,number,###########0.00} </text> and in RealBasicNoPutResultSetStatistics.dumpEstimatedCosts() from MessageService.getTextMessage(SQLState.RTS_OPT_EST_RC) + ": " + formatDouble(optimizerEstimatedRowCount) + "\n" + to MessageService.getTextMessage(SQLState.RTS_OPT_EST_RC, new Double(optimizerEstimatedRowCount)) + "\n" + ? Similar changes would be needed for RTS_OPT_EST_COST too. Then we avoid the need to explicitly create and cache a DecimalFormat instance, and the number will be formatted using the same locale as the rest of the message (the database locale). And we can remove the formatDouble() method, so we'll reduce the code size too.
        Hide
        Dag H. Wanvik added a comment -

        Thanks, folk! I had made a patch along the lines of Bryan's suggestion more or less, making the format non-static and initializing the db locale in the contructor. Knut's idea seemed attractive, however, and I made a try. It turns out the MessageBuilder chokes on it when it tries to generate DITA source, not being prepared for anything but

        {<string>}

        parameters in message strings in messages.xml: it tries to put the name of the meta-variable in the string, cf this:

        <row>
        <entry colname="col1">01006</entry>
        <entry colname="col2">Privilege not revoked from user <varname><authorizationID></varname>.</entry>
        </row>

        Obviously, trying to format a string like "<varname><number></varname>" with the format "

        {0,number,###########0.00}

        " doesn't work very well...

        I'll see it I can work around that somehow.

        Show
        Dag H. Wanvik added a comment - Thanks, folk! I had made a patch along the lines of Bryan's suggestion more or less, making the format non-static and initializing the db locale in the contructor. Knut's idea seemed attractive, however, and I made a try. It turns out the MessageBuilder chokes on it when it tries to generate DITA source, not being prepared for anything but {<string>} parameters in message strings in messages.xml: it tries to put the name of the meta-variable in the string, cf this: <row> <entry colname="col1">01006</entry> <entry colname="col2">Privilege not revoked from user <varname><authorizationID></varname>.</entry> </row> Obviously, trying to format a string like "<varname><number></varname>" with the format " {0,number,###########0.00} " doesn't work very well... I'll see it I can work around that somehow.
        Hide
        Dag H. Wanvik added a comment -

        Uploading patch #1 for this issue using Knut's suggestion. It leads to a small behavioral change: the leading blanks before the double decimals are gone, cf the logic in RealBasicNoPutResultSetStatistics#formatDouble (removed). I don't think we'll miss those, though.

        Running regressions.

        Show
        Dag H. Wanvik added a comment - Uploading patch #1 for this issue using Knut's suggestion. It leads to a small behavioral change: the leading blanks before the double decimals are gone, cf the logic in RealBasicNoPutResultSetStatistics#formatDouble (removed). I don't think we'll miss those, though. Running regressions.
        Hide
        Dag H. Wanvik added a comment -

        Renamed this issue to better reflect the problem.

        Show
        Dag H. Wanvik added a comment - Renamed this issue to better reflect the problem.
        Hide
        Knut Anders Hatlen added a comment -

        As to the renaming of the issue, isn't it rather RealBasicNoPutResultSetStatistics that's locale insensitive than the regression test suite being locale sensitive?

        The patch looks fine to me. Two possible improvements:

        • Should we append ": {0,number,###########0.00}

          " to the localized versions of these messages?

        • In MessageBuilder, it might be easier to clear the format specifier using MessageFormat.setFormatByArgumentIndex(). Something like:

        — a/java/build/org/apache/derbyBuild/MessageBuilder.java
        +++ b/java/build/org/apache/derbyBuild/MessageBuilder.java
        @@ -659,14 +659,16 @@ public class MessageBuilder extends Task
        {
        int count = rawArgs.length;
        String[] cookedArgs = new String[ count ];
        + MessageFormat format = new MessageFormat(message);

        // add xml angle brackets around the args
        for ( int i = 0; i < count; i++ )

        { cookedArgs[ i ] = "<varname><" + rawArgs[ i ] + "></varname>"; + format.setFormatByArgumentIndex(i, null); }
        • return MessageFormat.format( message, cookedArgs );
          + return format.format(cookedArgs);
          }

        /////////////////////////////////////////////////////////////////////////

        Show
        Knut Anders Hatlen added a comment - As to the renaming of the issue, isn't it rather RealBasicNoPutResultSetStatistics that's locale insensitive than the regression test suite being locale sensitive? The patch looks fine to me. Two possible improvements: Should we append ": {0,number,###########0.00} " to the localized versions of these messages? In MessageBuilder, it might be easier to clear the format specifier using MessageFormat.setFormatByArgumentIndex(). Something like: — a/java/build/org/apache/derbyBuild/MessageBuilder.java +++ b/java/build/org/apache/derbyBuild/MessageBuilder.java @@ -659,14 +659,16 @@ public class MessageBuilder extends Task { int count = rawArgs.length; String[] cookedArgs = new String[ count ]; + MessageFormat format = new MessageFormat(message); // add xml angle brackets around the args for ( int i = 0; i < count; i++ ) { cookedArgs[ i ] = "<varname><" + rawArgs[ i ] + "></varname>"; + format.setFormatByArgumentIndex(i, null); } return MessageFormat.format( message, cookedArgs ); + return format.format(cookedArgs); } /////////////////////////////////////////////////////////////////////////
        Hide
        Dag H. Wanvik added a comment -

        Regression results: lang/subqueryFlattening.sql failed because the exisiting output of an execution plan was cut short by being longer than 32672 characters. Since the new formatting saves some whitespace, it managed to print more of the execution plan and failed (the script somewhat misleadingly had set maxcolumndisplaywidth to 40000, over the max for the VARCHAR column metadata returned for "values SYSCS_UTIL.SYSCS_GET_RUNTIMESTATISTICS()".

        Show
        Dag H. Wanvik added a comment - Regression results: lang/subqueryFlattening.sql failed because the exisiting output of an execution plan was cut short by being longer than 32672 characters. Since the new formatting saves some whitespace, it managed to print more of the execution plan and failed (the script somewhat misleadingly had set maxcolumndisplaywidth to 40000, over the max for the VARCHAR column metadata returned for "values SYSCS_UTIL.SYSCS_GET_RUNTIMESTATISTICS()".
        Hide
        Dag H. Wanvik added a comment -

        Thanks Knut, yes, looks like a good simplification! As for the naming, I agree.

        Show
        Dag H. Wanvik added a comment - Thanks Knut, yes, looks like a good simplification! As for the naming, I agree.
        Hide
        Dag H. Wanvik added a comment -

        Attaching version #2, which makes the simplification suggested by Knut, fixes the master file of subqueryFlattening.sql and adjusts the other locales' message files. Running regressions over.

        Show
        Dag H. Wanvik added a comment - Attaching version #2, which makes the simplification suggested by Knut, fixes the master file of subqueryFlattening.sql and adjusts the other locales' message files. Running regressions over.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Dag. The patch looks good to me. Please also remove the added java.util.regex imports in MessageBuilder before you commit, since they aren't used in this version of the patch.

        Show
        Knut Anders Hatlen added a comment - Thanks, Dag. The patch looks good to me. Please also remove the added java.util.regex imports in MessageBuilder before you commit, since they aren't used in this version of the patch.
        Hide
        Dag H. Wanvik added a comment -

        Committed at svn 1176633 and 1176636, resolving.

        Show
        Dag H. Wanvik added a comment - Committed at svn 1176633 and 1176636, resolving.
        Hide
        Dag H. Wanvik added a comment -

        Backported to 10.8 branch as svn 1176939, closing. Note: the change to TableLockBasicTest on trunk was not backported since that test hasn't been backported.

        Show
        Dag H. Wanvik added a comment - Backported to 10.8 branch as svn 1176939, closing. Note: the change to TableLockBasicTest on trunk was not backported since that test hasn't been backported.

          People

          • Assignee:
            Dag H. Wanvik
            Reporter:
            Dag H. Wanvik
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development