Bugzilla – Full Text Bug Listing
| Summary: | Improve Extendability of the SQL Query Source Writer | ||
|---|---|---|---|
| Product: | [Tools] Data Tools | Reporter: | Linda Chan <lchan> |
| Component: | ModelBase | Assignee: | Brian Payton <bpayton> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bpayton |
| Version: | 1.8 | Flags: | lchan:
review?
(bpayton) |
| Target Milestone: | 1.8.1 | ||
| Hardware: | PC | ||
| OS: | Windows Server 2003 | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 175123 [details]
Revised patch
Further refactoring to allow an extended SQLQuerySourceWriter to override the call to StatementHelper#convertCatalogIdentifierToSQLFormat.
Created attachment 175521 [details]
Revised Patch
Removed the prior changes to use ISQLObjectNameHelper.
The only source changes left in this patch is the extraction to the call of StatementHelper#convertCatalogIdentifierToSQLFormat into a protected method.
This will hopefully expedite the review process.
Hi Linda, I took this opportunity to clean up the methods in the SQLQuerySourceWriter that are being changed. Over time I've been trying to make the class easier to subclass. For example, I've been changing sb.append(xxx) method calls to appendYyyy(sb, xxx), so that extenders can get control when each language element is written out (for example, to change the case of some language elements). I added appendDataTypeName, appendFunctionName, appendIdentifier methods. I've also been refactoring the code to make it easier to debug by splitting up nested method calls. Created attachment 175529 [details]
Patch for plugin o.e.d.modelbase.sql.query
Linda, please try out this patch to make sure I didn't mess anything up. I ran all the parser JUnit test cases, and they ran OK.
(In reply to comment #4) Brian, The additional re-factoring is a great idea. The latest patch seems to work fine for me too. Thanks. Brian, I find that the new method #appendOperator(StringBuffer sb, String oper) is not called. Shouldn't it be called by #appendSpecificSQL(ValueExpressionCombinedOperator op, StringBuffer sb) ? I added appendOperator quite a while ago, not sure why I didn't use it yet. I'll do some more updates. Created attachment 175536 [details]
Patch for plugin o.e.d.modelbase.sql.query
OK, I started going through and converting all the appendSpecificSQL methods to use the override-able appendXxxx methods. I converted a lot of them, but there's a lot left to do still. I'm going to go ahead and check in the fix, and carry on refactoring the methods later.
Checked in fix, tagged to v201007301145. Created attachment 175735 [details]
Further Update to call #appendOperator
The attached patch changes the #appendSpecificSQL(ValueExpressionCombinedOperator , StringBuffer) method to call #appendOperator for the combined operator string.
Brian, This additional patch adds re-factoring of #appendSpecificSQL(ValueExpressionCombinedOperator, StringBuffer) to call #appendOperator. Created attachment 175899 [details]
Another patch for plugin o.e.d.modelbase.sql.query
Sorry, was going through the code refactoring all the sb.append method calls, and got tired before reaching that one.
With this patch I've completed the refactoring and other code cleanup.
Checked in patch, tagged to v201008050645. Let me know if you see any problems. Created attachment 175991 [details]
Fix calls to #appendSpace(sb, SPACE)
Brian,
I found a few of these calls are now made: appendSpace(sbSelect, SPACE),
which led to adding 32 spaces.
These were probably typos from recent re-factoring.
See patch that identifies those calls.
Thanks, I was trying to watch out for those, but I must have been getting sleepy. (It's pretty tedious going through all that code.) It's annoying that the char ' ' is accepted as a integer argument. It's a legacy of Java's "C" roots, I think. Checked in fix, tagged to v201008060930. Created attachment 175997 [details]
Refactoring in #appendSQLForTableInDatabase(TableInDatabase, StringBuffer)
Just a bit more re-factoring...
This patch makes separate calls to #appendIdentifier for the table qualifier and table name, so that the DOT separator is handled by #appendSymbol.
Created attachment 176065 [details]
Refactoring in #appendSQLForTableInDatabase and for OrderByResultColumn
Brian,
Including another minor re-factoring for
#appendSpecificSQL(OrderByResultColumn, StringBuffer)
OK, I applied the patch from 2010-08-06 with the refactoring of appendSQLforTableInDatabase and appendSpecificSQL(OrderByResultColumn..., tagged to v201008100700. Resolving as fixed. |
Created attachment 174550 [details] Proposed Patch The schema of the extension point org.eclipse.datatools.modelbase.sql.query.sourceWriterExtension has incorrect base class value in the 2 class attributes. See attached patch for the fix. Separately, it would be nice for the default SQLQuerySourceWriter to let an ISQLObjectNameHelper, if available, to determine the SQL format of the table name written in the generated SQL query text. See patch for enhanced implementation.