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 <[EMAIL PROTECTED]> wrote:
Hi,
I wondered if anyone got a chance to review this patch?
thanks,
Mamta
On 4/24/05, Mamta Satoor <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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.
