You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/03/07 13:03:28 UTC

[GitHub] [shardingsphere] tristaZero commented on pull request #9599: Add `MERGE` statement test case

tristaZero commented on pull request #9599:
URL: https://github.com/apache/shardingsphere/pull/9599#issuecomment-792275813


   Hi @ThanoshanMV ,
   
   The PR looks almost correct. Nice!
   Actually, it is challenging to visit out the corresponding `SQLStatement`. Anyway, we can firstly fill some necessary `Segment`s into the target `SQLStatement`.
   
   Here is another issue needing your attention.
   
   Could you figure out whether there are similar `MERGE` SQL definitions in other databases, like `SQLServer`, `SQL92`?
   
   The reason is that I found you created an abstract class, `MergeStatement`. Generally, an abstract class `MergeStatement` is designed to imply that there will be many final child classes from various databases, like the `OracleMergeStatement` class from you.
   
   So if `MERGE` SQL is specific to `Oracle`, it is suggested to remove abstract `MergeStatement`.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org