On 2016/11/04 13:08, Ashutosh Bapat wrote:
> On Fri, Nov 4, 2016 at 9:19 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2016/11/03 18:52, Ashutosh Bapat wrote:
I wrote:
>>>> Here is the updated version,
>>>> Other than the above issue and the alias issue we discussed, I addressed
>>>> all
>>>> your comments except one on testing; I tried to add test cases where the
>>>> remote query is deparsed as nested subqueries, but I couldn't because
>>>> IIUC,
>>>> reduce_outer_joins reduced full joins to inner joins or left joins.
>>> No always. It will do so in some cases as explained in the prologue of
>>> reduce_outer_joins()
>>>
>>> * More generally, an outer join can be reduced in strength if there is a
>>> * strict qual above it in the qual tree that constrains a Var from the
>>> * nullable side of the join to be non-null. (For FULL joins this applies
>>> * to each side separately.)
Right.
>> Would it be necessary for this patch to include test cases for nested
>> subqueries?
> A patch should have testcases testing the functionality added. This
> patch adds functionality to deparse nested subqueries, so there should
> be testcase to test it.
OK, I added such a test case.
>>> This patch again has a lot of unrelated changes, esp. in
>>> deparseSelectStmtForRel(). What was earlier part of deparseSelectSql()
>>> and deparseFromExpr() is now flattened in deparseSelectStmtForRel(),
>>> which seems unnecessary.
>> IIUC, I think this change was done to address your comment (see the comment
>> #2 in [1]). Am I missing something?
> There is some misunderstanding here. That comment basically says,
> deparseRangeTblRef() duplicates code in deparseSelectStmtForRel(), so
> we should either remove deparseRangeTblRef() altogether or should keep
> it to minimal avoiding duplication of code. What might have confused
> you is the last sentence in that comment "This will also make the
> current changes to deparseSelectSql unnecessary." By current changes I
> meant changes to deparseSelectSql() in your patch, not the one that's
> in the repository. I don't think we should flatten
> deparseSelectStmtForRel() in this patch.
I noticed that I misunderstood your words. Sorry about that. I agree
with you, so I removed that change from the patch.
Attached is an updated version of the patch.
Best regards,
Etsuro Fujita