db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mamta Satoor <msat...@gmail.com>
Subject Re: [PATCH] Jira-189 ResultSetMetaData.getSchemaName and ResultSetMetaData.isWritable donot return correct values
Date Tue, 03 May 2005 21:34:25 GMT
Hi David,

I have attached a new patch to take care of the comments from your review.

1)Good catch about
impl.sql.compile.ResultColumnList.getResultColumnThroughExpression().
It is left over code from another solution I was trying to fix the
problem. I have taken it out of the review package attached to this
mail.

2)The change in ResultColumnDescriptor.java for getSourceSchemaName is
for clarity and consistency reasons (similar to getSourceTableName).
With this
change, EmbedResultSetMetaData now calls getSourceXXXName for
getTableName and getSchemaName. Similarly, in
GenericColumnDescriptor(which implements ResultColumnDescriptor), we
have getSourceXXXName to keep the source table and source schema name.

3)I have reformatted code and comments to not exceed 80 characters.
Also, changed the comments for method commonCodeForUpdateableBySql
from .. to /** and */

thanks,
Mamta
On 5/2/05, David Van Couvering <David.Vancouvering@sun.com> wrote:
> Hi, Mamta, this looks good.  It built, and I ran as many tests as I
> could before I had to leave for the day.  My God, those tests take a
> long time to run.  But that's a topic for a separate discussion :)
> 
> Here are my comments and questions...
> 
> - What you have looks good and makes sense, but I don't know the code
> well enough to know if you're missing something.  I guess the tests help
> guarantee that, but someone more familiar with the code taking a look
> would be great.
> 
> - I was curious how
> impl.sql.compile.ResultColumnList.getResultColumnThroughExpression() was
> used.  In NetBeans I searched for all usages of it and could find none.
>   To double-check, I did a full search of the codeline and only found
> it where it is defined, and no uses.  Is there a reason to have this
> method if nothing uses it?
> 
> - in ResultColumnDescriptor.java, why did you change the method from
> getSchemaName() to getSourceSchemaName()?  Is this just to make it more
> clear what the method is doing?
> 
> - A lot of your comments and code seem to far exceed 80 characters in
> length per line.  This makes it hard to read in diff output and
> text-based editors, and it requires scrolling in IDEs.  Could you please
> limit your source lines to 80 characters?
> 
> - The method commonCodeForUpdateableBySql in ResultColumnList.java
> should have comments with /** and */ rather than //  It took me a while
> in 'vi' to determine this was a new method, to the eye it looks like a
> continuation of the previous method.
> 
> David
> 
> Mamta Satoor wrote:
> 
> > Hi,
> >
> > I am sorry if I sound like a broken record, but please, will someone
> > review this? And if no concerns from anyone, will a commiter commit
> > it?
> >
> > thanks,
> > Mamta
> >
> > On 4/27/05, Mamta Satoor <msatoor@gmail.com> wrote:
> >
> >>Hi,
> >>
> >>I wondered if anyone got a chance to review this patch?
> >>
> >>thanks,
> >>Mamta
> >>
> >>On 4/24/05, Mamta Satoor <msatoor@gmail.com> wrote:
> >>
> >>>Hi Dan,
> >>>
> >>>I have a new patch for this bug which also fixes the problem you
> >>>brought up with sql select * from a.t as X. The fix for this required
> >>>change in impl.sql.compile.FromBaseTable's method genResultColList().
> >>>I changed the code such that we set the TableDescriptor on the
> >>>ColumnDescriptor instance. This TableDescriptor is later used by
> >>>ResultColumn.getTableName to get the base table name of the column. In
> >>>addition to that, I changed ColumnReference.getSourceTableName and
> >>>ColumnReference.getSourceSchemaName so that they don't look at the
> >>>user supplied correlation name (if any) to fetch the base table/schema
> >>>name.
> >>>
> >>>I have also added some test cases for the code changes above in
> >>>jdbcapi/resultset.java
> >>>
> >>>Other than this, the rest of the patch stays the same as what you
> >>>tried applying last week.
> >>>
> >>>I have run the tests and there is no new failure because of my changes.
> >>>
> >>>svn stat output
> >>>M      java\engine\org\apache\derby\impl\sql\compile\ResultColumn.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\VirtualColumnNode.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\CursorNode.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\FromBaseTable.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\BaseColumnNode.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\ColumnReference.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\ValueNode.java
> >>>M      java\engine\org\apache\derby\impl\sql\compile\ResultColumnList.java
> >>>M      java\engine\org\apache\derby\impl\sql\GenericColumnDescriptor.java
> >>>M      java\engine\org\apache\derby\impl\jdbc\EmbedResultSet.java
> >>>M      java\engine\org\apache\derby\impl\jdbc\EmbedResultSetMetaData.java
> >>>M      java\engine\org\apache\derby\iapi\sql\dictionary\ColumnDescriptor.java
> >>>M      java\engine\org\apache\derby\iapi\sql\ResultColumnDescriptor.java
> >>>M      java\testing\org\apache\derbyTesting\functionTests\tests\lang\updatableResultSet.java
> >>>M      java\testing\org\apache\derbyTesting\functionTests\tests\jdbcapi\resultset.java
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\DerbyNet\updatableResultSet.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\DerbyNet\resultset.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\DerbyNetClient\updatableResultSet.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\DerbyNetClient\resultset.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\updatableResultSet.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\jdk14\updatableResultSet.out
> >>>M      java\testing\org\apache\derbyTesting\functionTests\master\resultset.out
> >>>
> >>>Please commit the patch if there are no further issues,
> >>>Mamta
> >>>
> >>>On 4/22/05, Mamta Satoor <msatoor@gmail.com> wrote:
> >>>
> >>>>Hi Dan,
> >>>>
> >>>>I did some research into this. You are right that for the sql
> >>>>select * from a.t as X
> >>>>the existing code will return ABC using both getTableName and
> >>>>getSourceTableName. Looking at the history of ColumnReference before
> >>>>the code was
> >>>>contributed, the getSourceTableName was added to Cloudscape so that
> >>>>ResultSetMetaData.getTableName will return the correct value which is
> >>>>base table
> >>>>name. Seems like over the time, this functionality got broken again.
> >>>>
> >>>>In my code line, I tried changing ColumnReference.getSourceTableName
> >>>>to following
> >>>>public String getSourceTableName()
> >>>>{
> >>>>return ((source != null) ? source.getTableName() : null);
> >>>>}
> >>>>
> >>>>But, that did not solve the problem. The source.getTableName() invokes
> >>>>ResultColumn.getTableName which is currently coded as follows
> >>>>public String getTableName()
> >>>>{
> >>>> if (tableName != null)
> >>>> {
> >>>>   return tableName;
> >>>> }
> >>>> if ((columnDescriptor!=null) &&
> >>>>   (columnDescriptor.getTableDescriptor() != null))
> >>>> {
> >>>>   return columnDescriptor.getTableDescriptor().getName();
> >>>> }
> >>>> else
> >>>> {
> >>>>   return expression.getTableName();
> >>>> }
> >>>>}
> >>>>
> >>>>In the code above, the first 2 if conditions return false and hence
> >>>>expression.getTableName() get called which returns ABC for the sql
> >>>>above. And this
> >>>>is the problem in my opinion. The 2nd if condition needs to succeed in
> >>>>order to get the correct value for table name(currently, the 2nd if
> >>>>condition
> >>>>returns false because columnDescriptor.getTableDescriptor() returns
> >>>>null). columnDescriptor has its table descriptor set to null since it
> >>>>got
> >>>>instantiated via SYSCOLUMNSRowFactory which passes the uuid for the
> >>>>table but not the table descriptor. I tried changing
> >>>>ColumnDescriptor's
> >>>>2nd constructor(the one which doesn't get table descriptor passed to
> >>>>it) to try to get the table descriptor from the uuid by calling
> >>>>getDataDictionary.getTableDescriptor(uuid), but it gives me Raw Store
> >>>>internal error. I will continue to research but does someone looking
> >>>>at this
> >>>>explanation have any thoughts on the program flow or issue in general?
> >>>>
> >>>>thanks,
> >>>>Mamta
> >>>>
> >>>>
> >>>>On 4/20/05, Daniel John Debrunner <djd@debrunners.com> wrote:
> >>>>
> >>>>>The patch applies fine and passes the tests but I need some
> >>>>>clarification on some methods in ColumnReference.
> >>>>>
> >>>>>ColumnReference has these class specific methods
> >>>>>
> >>>>>getSourceTableName()
> >>>>>getSourceSchemaName() [added by this contribution]
> >>>>>
> >>>>>and because it is a ValueNode it also has
> >>>>>
> >>>>>getTableName
> >>>>>getSchemaName
> >>>>>
> >>>>>Mamta, can you clarify the difference between the getSource*Name
methods
> >>>>>and get*Name methods? Eventually as comments in the javadoc for these
> >>>>>methods, but some discussion on the list may be useful.
> >>>>>
> >>>>>I'm worried because most of the bugs around correlation names I think
> >>>>>were due to having multiple methods with similar and maybe misleading
> >>>>>names but no clear definition of what they returned.
> >>>>>
> >>>>>In this specific case, my gut reaction from the names of the
> >>>>>getSource*Name methods is that they return the actual name of the
> >>>>>underlying table the column comes from. But looking the implementation
&
> >>>>>comments of getTableName and getSourceTableName in ColumnReference.java
> >>>>>it seems in this case
> >>>>>
> >>>>>select * from a.t as X
> >>>>>
> >>>>>that both methods will return X.
> >>>>>
> >>>>>Dan.
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> 
>

Mime
View raw message