Re: New CORRESPONDING clause design

Hi

2017-03-25 13:41 GMT+01:00 Surafel Temesgen <surafel3000(at)gmail(dot)com>:

>
>
>>
>> I took a quick look through this and noted that it fails to touch
>> ruleutils.c, which means that dumping of views containing CORRESPONDING
>> certainly doesn't work.
>>
> fixed
>
>> Also, the changes in parser/analyze.c seem rather massive and
>> correspondingly hard to review. Is it possible to rearrange the
>> patch to reduce the size of that diff? If you can avoid moving
>> or reindenting existing code, that'd help.
>>
> Part of transformSetOperationTree that make union data type of
> set operation target list became makeUnionDatatype inorder to
> easy using it multiple time and avoid very long transformSetOperationTree
> function
>
>
>> The code in that area seems rather confused, too. For instance,
>> I'm not sure exactly what orderCorrespondingList() is good for,
>> but it certainly doesn't look to be doing anything that either its
>> name or its header comment (or the comments at the call sites) would
>> suggest. Its input and output tlists are always in the same order.
>>
>> It give corresponding target list a sequential resnos
> Inorder to avoid touching generate_append_tlist I change
> the comment and function name as such
>
> I also think there should be some comments about exactly what matching
>> semantics we're enforcing. The SQL standard says
>>
>> a) If CORRESPONDING is specified, then:
>> i) Within the columns of T1, equivalent <column name>s shall
>> not be specified more than once and within the columns of
>> T2, equivalent <column name>s shall not be specified more
>> than once.
>>
>> That seems unreasonably stringent to me; it ought to be sufficient to
>> forbid duplicates of the names listed in CORRESPONDING, or the common
>> column names if there's no BY list. But whichever restriction you prefer,
>> this code seems to be failing to check it --- I certainly don't see any
>> new error message about "column name "foo" appears more than once".
>>
> fixed
>
> I'm not impressed by using A_Const for the members of the CORRESPONDING
>> name list. That's not a clever solution, that's a confusing kluge,
>> because it's a complete violation of the meaning of A_Const. Elsewhere
>> we just use lists of String for name lists, and that seems sufficient
>> here. Personally I'd just use the existing columnList production rather
>> than rolling your own.
>>
> fixed
>
>>
>>
I am sending updated version:

1. the corresponding columns must be unique, other not - code refactoring
(the code is still O(N*M*Z) - but some slow operations (string cmp)
reduced) + regress tests
2. regress tests for views
3. some cleaning (white chars)

Regards

Pavel