db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Van Couvering <David.Vancouver...@Sun.COM>
Subject Re: [PATCH] Jira-189 ResultSetMetaData.getSchemaName and ResultSetMetaData.isWritable donot return correct values
Date Mon, 02 May 2005 23:25:32 GMT
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