Implement Joins with multiple trailing ON Expressions #1303
Conversation
Fixes JSQLParser#1302 Fixes SpecialOracleTest JOIN17, now 190/273 tests pass
|
|
||
| @Override | ||
| public String toString() { | ||
| return appendTo(new StringBuilder()).toString(); |
There was a problem hiding this comment.
Again. Whats the improvement of this type of generating a StringBuilder and populating it? IMHO this is no improvement over this older toString approach.
There was a problem hiding this comment.
Good Morning. Thank you for your feedback.
My thought process was like this:
- I have had to touch
toString()because of the new Loop through the ON-Expressions - String Addition
+=in Loops is still considered bad style and will be called out by many style checkers, because the Compiler won't substitute the concatenation by a StringBuilder - providing an
appendTo()method consistently for all Statements and Expressions would eventually allow the Deparser to work with one single StringBuilder only (initiated at the very top). Right now, many Statements and Expressions initiate and dispose their own StringBuilder, putting useless load onto the GC (think about deep nested expressions especially). - I do not see any disadvantages of this pattern.
toString()still works without any caveats. We just delegate the logic into a special method. You did the same for many other Statements and Expressions already -- just not calling itappendTo()and not applying a systematic pattern yet.
What counter arguments do you see? Why was the String concatenation better, especially with the newly needed Loops?
Cheers.
There was a problem hiding this comment.
The main purpose of the deparsers is to provide a visitor type of working through the object tree and creating the string representation of the actual expression. If the deparser is somehow calling the toString method it is simply wrong or the result of lazyness. :(
There was a problem hiding this comment.
In my opinion, for the Leaves of the object tree, you will call toString(), or better appendTo().
You would walk through the Branches via the Visitor though.
Also, I have been using the term "De-parsing" rather lax, meaning both the toString() implementation as well as the Deparser implementation. I should have referred to it as "String Composition" instead.
When you call toString() on a complex, deeply nested Statement right now, you will create many instances of Strings and StringBuilder right now, often in loops.
With the appendTo() method, only one StringBuilder would be needed and I am eager to learn about the counter arguments against this approach.
There was a problem hiding this comment.
I have removed the appendTo() method and re-implemented the traditional toString() as you obviously do not like this approach.
…sLast � Conflicts: � src/test/java/net/sf/jsqlparser/statement/select/SelectTest.java � src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/connect_by01.sql � src/test/resources/net/sf/jsqlparser/statement/select/oracle-tests/connect_by06.sql
Refactor the appendTo() method in favour of the traditional toString()
Fixes #1302
Fixes #1229
Fixes SpecialOracleTest JOIN17, now 190/273 tests pass