db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Satheesh Bandaram <sathe...@Sourcery.Org>
Subject Re: [PATCH] (DERBY-104) Get rid of the Max lenght of 18 for constraint names
Date Thu, 12 May 2005 22:12:35 GMT
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
  <title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Hi Bernt,<br>
<br>
I tried to apply your patch. Most of the patch applied correctly,
except for sqlgrammar.jj. There have been other modifications to
sqlgrammar.jj since the patch was made. I could either try to fix them
up or ask you to submit another patch. It may be best if you submit a
new patch, based on Friday evening code. I will apply the patch and run
tests in my environment to catch any major failures. Then I will submit
it on Saturday morning, if the plan works for you.<br>
<br>
You can decide if you would like to change the names of DB2 specific
constant names being discussed here. I am OK eitherway.<br>
<br>
Satheesh<br>
<br>
Bernt M. Johnsen wrote:
<blockquote cite="mid20050512001524.GC13782@ac30.fr34.2y.net"
 type="cite">
  <pre wrap="">See comments inline.

  </pre>
  <blockquote type="cite">
    <blockquote type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <blockquote type="cite">
              <blockquote type="cite">
                <blockquote type="cite">
                  <blockquote type="cite">
                    <blockquote type="cite">
                      <blockquote type="cite">
                        <blockquote type="cite">
                          <pre wrap="">Army wrote (2005-05-11 16:18:42):
                          </pre>
                        </blockquote>
                      </blockquote>
                    </blockquote>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
      </blockquote>
    </blockquote>
    <pre wrap="">Bernt M. Johnsen wrote:

    </pre>
    <blockquote type="cite">
      <pre wrap="">Hi all,

Could somebody review this patch (my first)? Now all identifiers
should have a max length of 128 (note: MAX_USERID_LENGTH is
unchanged).
      </pre>
    </blockquote>
    <pre wrap="">I reviewed this patch and it looks good to me.  I noticed that in the
JIRA 
entry for this bug, you had written that you wanted to do the following:

[  begin quote ]

2) Ensure that all DB2 related constants are prefixed by DB2_
   There are now 6 constants which do not have the prefix: 
MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, 
MAX_USERID_LENGTH, MAX_DECIMAL_PRECISION_SCALE, DEFAULT_DECIMAL_PRECISION, 
DEFAULT_DECIMAL_SCALE
These should get the DB2_ prefix if they are DB2-related (are they?)

[ end quote ]

Did you decide to not do this part?  
    </pre>
  </blockquote>
  <pre wrap=""><!---->
I never got an answer, so I decided not to touch these constants.

  </pre>
  <blockquote type="cite">
    <pre wrap="">For the record, the comments in the 
code suggest that the first three of these 
(MIN_COL_LENGTH_FOR_CURRENT_USER, MIN_COL_LENGTH_FOR_CURRENT_SCHEMA, and 
MAX_USERID_LENGTH) are definitely DB2-related, so adding the "DB2_" prefix 
to those would make sense.  
    </pre>
  </blockquote>
  <pre wrap=""><!---->
Should I add "DB2_"-prefix to these constants, then? (and leave the
latter-three unchanged?).

  </pre>
  <blockquote type="cite">
    <pre wrap="">As for the latter three, one might assume that 
they are DB2-specific since they were declared in the "DB2Limits.java" 
file, but maybe that's not a valid assumption...anyone out there know??  On 
the one hand, I think these "DB2_" prefix changes could easily be checked 
in later with a different patch, if someone wants to do it.  On the other, 
it might be less inuitive to people browsing changes to see a solo patch 
that adds a "DB2_" prefix to some constants in "Limits.java"--at the very 
least, the patch to do so would have to be very well-commented, or else it 
could lead to confusion.

    </pre>
    <blockquote type="cite">
      <pre wrap="">NOTE: I couldn't get svn diff to reflect that I have renamed one
file
(done with svn rename). If it is a problem, I can undo the rename and
submit a new patch.
      </pre>
    </blockquote>
    <pre wrap="">I did have problems applying the patch to the my local codeline, and
I'm 
pretty sure it was because of this.  In order to get the patch to apply, I 
manually copied "DB2Limit.java" to "Limits.java"--after doing that (and 
only that), I was then able to successfully apply the patch.  So whoever 
commits this might need to do the same, or else use some other trick to get 
the patch to apply correctly.
    </pre>
  </blockquote>
  <pre wrap=""><!---->
Seems to me that this is a weakness in the way we submit pathces
(probably a weakness in svn).

  </pre>
  <blockquote type="cite">
    <pre wrap="">After applying the patch I ran a couple of the tests that were affected,

and they passed as expected.

One last question: did you have a chance to run the "derbyall" suite on 
this to make sure you caught all of the relevant tests?  I'm guessing that 
you did based on the number of masters that you updated, but it'd be nice 
to confirm--typically when you submit a patch, it's good to indicate what 
tests you ran (and on what platforms/JVMs) just so that reviewers know you 
actually tested your patch before posting it (which you clearly
did!) ;)
    </pre>
  </blockquote>
  <pre wrap=""><!---->
Yes, I ran derbyall:

Java Version:    1.4.2_02
Java Vendor:     Sun Microsystems Inc.
OS name:         Linux
OS architecture: i386
OS version:      2.4.20-31.9

  </pre>
  <blockquote type="cite">
    <pre wrap="">All in all, looks good to me.
    </pre>
  </blockquote>
  <pre wrap=""><!---->
Thanks.

  </pre>
</blockquote>
</body>
</html>


Mime
View raw message