Reported type mismatch improperly

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Reported type mismatch improperly

Andy Fan
Currently when we call select_common_type, it compares the 2 exprs, if the expr
type of both are unknown, it will be set to TEXTOID with some reasons, which
can cause the issue like below.


postgres=# select null union all select null union all select 1;
ERROR:  UNION types text and integer cannot be matched
LINE 1: select null union all select null union all select 1;


In this case, we can't blame the user,  they may want the nulls to be at the top
of the result. 

I worked on a patch to fix this, the main idea is before going to the above
logic, I peak all the exprs for a given column first, and choose a default one
when we see the Unknown & Unknown case(rather than TextOid),  

do you think it is ok?

--
Best Regards
Andy Fan

v1-0001-Detect-the-null-type-correctly-in-setop-case-by-p.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reported type mismatch improperly

David G Johnston
On Fri, Jul 17, 2020 at 2:00 AM Andy Fan <[hidden email]> wrote:
Currently when we call select_common_type, it compares the 2 exprs, if the expr
type of both are unknown, it will be set to TEXTOID with some reasons, which
can cause the issue like below.


postgres=# select null union all select null union all select 1;
ERROR:  UNION types text and integer cannot be matched
LINE 1: select null union all select null union all select 1;


In this case, we can't blame the user,  they may want the nulls to be at the top
of the result. 

This seems like a win in terms of benefit for effort.  Today we just instruct users to cast the first null - which they still need to be aware of even though this patch removes one situation where it matters.


I worked on a patch to fix this, the main idea is before going to the above
logic, I peak all the exprs for a given column first, and choose a default one
when we see the Unknown & Unknown case(rather than TextOid),  

do you think it is ok?

The comment in parse_coerce.c needs to be updated.

I'm not sure what the general protocol here is for code comments and pointing out that they are not good examples of English prose.  Documentation prose has a higher bar but I don't know how big the gap is.

make issues: variable previous_is_null set but not used warning

make installcheck passes

You should include tests for the other SETOPS if this is intended to work for those as well.  And maybe the failure mode where there are two non-null possibilities in the tree and the first one matches while the second is of a different type.

Nothing in the code itself stands out to me and I'm not going to mark this ready for committer myself (though its not ready yet per the remarks above) due to inexperience in C coding so another reviewer will be needed.

You should add this to the 2020-09 commitfest.

David J.

Reply | Threaded
Open this post in threaded view
|

Re: Reported type mismatch improperly

Andy Fan


On Sat, Jul 18, 2020 at 12:23 AM David G. Johnston <[hidden email]> wrote:
On Fri, Jul 17, 2020 at 2:00 AM Andy Fan <[hidden email]> wrote:
Currently when we call select_common_type, it compares the 2 exprs, if the expr
type of both are unknown, it will be set to TEXTOID with some reasons, which
can cause the issue like below.


postgres=# select null union all select null union all select 1;
ERROR:  UNION types text and integer cannot be matched
LINE 1: select null union all select null union all select 1;


In this case, we can't blame the user,  they may want the nulls to be at the top
of the result. 

This seems like a win in terms of benefit for effort.  Today we just instruct users to cast the first null

I think you are saying about  "select null::init",  I forget this before.. I agree 
with the "benefit for effort" judgment,  This would be good if it can remove
some "tech debt" and without introducing new problems. 
 
- which they still need to be aware of even though this patch removes one situation where it matters.

Can you provide an example of this?  If it is not very troublesome,  I'd like to fix both.



I worked on a patch to fix this, the main idea is before going to the above
logic, I peak all the exprs for a given column first, and choose a default one
when we see the Unknown & Unknown case(rather than TextOid),  

do you think it is ok?

The comment in parse_coerce.c needs to be updated.

I'm not sure what the general protocol here is for code comments and pointing out that they are not good examples of English prose.  Documentation prose has a higher bar but I don't know how big the gap is.

make issues: variable previous_is_null set but not used warning

make installcheck passes

You should include tests for the other SETOPS if this is intended to work for those as well.  And maybe the failure mode where there are two non-null possibilities in the tree and the first one matches while the second is of a different type.

Nothing in the code itself stands out to me and I'm not going to mark this ready for committer myself (though its not ready yet per the remarks above) due to inexperience in C coding so another reviewer will be needed.

You should add this to the 2020-09 commitfest.
 

The above patch is just to see if there are any objections.  I'd try to fix the
above issues on the official patch.  Thank you for your feedback!

--
Best Regards
Andy Fan
Reply | Threaded
Open this post in threaded view
|

Re: Reported type mismatch improperly

David G Johnston
On Friday, July 17, 2020, Andy Fan <[hidden email]> wrote:


On Sat, Jul 18, 2020 at 12:23 AM David G. Johnston <[hidden email]> wrote:

- which they still need to be aware of even though this patch removes one situation where it matters.

Can you provide an example of this?  If it is not very troublesome,  I'd like to fix both.

Nothing concrete off hand, more of an impression.

David J.
Reply | Threaded
Open this post in threaded view
|

Re: Reported type mismatch improperly

Andy Fan


On Sat, Jul 18, 2020 at 12:28 PM David G. Johnston <[hidden email]> wrote:
On Friday, July 17, 2020, Andy Fan <[hidden email]> wrote:


On Sat, Jul 18, 2020 at 12:23 AM David G. Johnston <[hidden email]> wrote:

- which they still need to be aware of even though this patch removes one situation where it matters.

Can you provide an example of this?  If it is not very troublesome,  I'd like to fix both.

Nothing concrete off hand, more of an impression.

David J.

Improve some comments and test cases, and submit it https://commitfest.postgresql.org/29/2653/ . 


--
Best Regards
Andy Fan

v2-0001-Detect-the-null-type-correctly-in-setop-case-by-p.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reported type mismatch improperly

Andy Fan


On Sun, Jul 19, 2020 at 10:25 AM Andy Fan <[hidden email]> wrote:


On Sat, Jul 18, 2020 at 12:28 PM David G. Johnston <[hidden email]> wrote:
On Friday, July 17, 2020, Andy Fan <[hidden email]> wrote:


On Sat, Jul 18, 2020 at 12:23 AM David G. Johnston <[hidden email]> wrote:

- which they still need to be aware of even though this patch removes one situation where it matters.

Can you provide an example of this?  If it is not very troublesome,  I'd like to fix both.

Nothing concrete off hand, more of an impression.

David J.

Improve some comments and test cases, and submit it https://commitfest.postgresql.org/29/2653/ . 

I have withdrawn the patch in commitfest since the current solution is bad. To fix it I have
to spend more time on tranformExpr part.  I can open a new one if I get it really fixed.  Now 
I have let the user cast the null to int explicitly. 

--
Best Regards
Andy Fan