Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Union columns with different types #3467

Closed
gandronchik opened this issue Sep 13, 2022 · 8 comments · Fixed by #3513
Closed

Union columns with different types #3467

gandronchik opened this issue Sep 13, 2022 · 8 comments · Fixed by #3513
Labels
bug Something isn't working

Comments

@gandronchik
Copy link
Contributor

gandronchik commented Sep 13, 2022

Describe the bug
You can union columns with different types. And it crashes if we order by this column

To Reproduce
it is just strange behavior:
select 1 a union all select 'a';

+---+
| a |
+---+
| 1 |
| a |
+---+

it should work, however since 1 is an integer and 1.1 is a decimal, it crashes:
select 1 a union all select 1.1 order by 1;

Additional context
Physical Sort Node does concat and it crashes. Looks like Union node has to autocast numeric types and returns error for rest types;

@gandronchik gandronchik added the bug Something isn't working label Sep 13, 2022
@gandronchik
Copy link
Contributor Author

@alamb I am going to fix this problem. Is it fine if I will implement autocast for numeric types in UnionExec? Or you see better way?

@alamb
Copy link
Contributor

alamb commented Sep 13, 2022

@alamb I am going to fix this problem. Is it fine if I will implement autocast for numeric types in UnionExec? Or you see better way?

I think the appropriate code to call would be "coerce" (as in the types of the union should be coerced to a common type). There is already code to do that for other similar operations (like InList, etc)

@liukun4515
Copy link
Contributor

@gandronchik
This is an error in the data type or type coercion which is not supported in the union.
You can follow the inlist or the binary type coercion do this in the union, we can help you to review the pr.

But now we are doing the migration the type coercion to logical phase #3388

@gandronchik
Copy link
Contributor Author

@liukun4515 Got it. I will implement it in logical plan then

@liukun4515
Copy link
Contributor

liukun4515 commented Sep 14, 2022

@liukun4515 Got it. I will implement it in logical plan then

you can refer this pr #3472

@gandronchik
Copy link
Contributor Author

@liukun4515 Got it. I will implement it in logical plan then

you can refer this pr #3472

Why did you decide to do coercion in optimizators? At least in case with union I think it is better to do it directly in union building function

pub fn union_with_alias(
    left_plan: LogicalPlan,
    right_plan: LogicalPlan,
    alias: Option<String>,
) -> Result<LogicalPlan> {

@ovr
Copy link
Contributor

ovr commented Sep 16, 2022

I highly agree with @gandronchik that this should be done in the planner instead of the type coerce optimizer.

@liukun4515
Copy link
Contributor

@liukun4515 Got it. I will implement it in logical plan then

you can refer this pr #3472

Why did you decide to do coercion in optimizators? At least in case with union I think it is better to do it directly in union building function

pub fn union_with_alias(
    left_plan: LogicalPlan,
    right_plan: LogicalPlan,
    alias: Option<String>,
) -> Result<LogicalPlan> {

Yes, agree with you.

The type coercion should be done in the analyse phase, because of the codebase we can do this in the right place.

We do the type coercion in the physical phase when creating the physical expr.

cc @alamb @andygrove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants