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

Feature: TransformCastSchema need check load_can_auto_cast_to during construction #16790

Open
sundy-li opened this issue Nov 7, 2024 · 6 comments · May be fixed by #16836
Open

Feature: TransformCastSchema need check load_can_auto_cast_to during construction #16790

sundy-li opened this issue Nov 7, 2024 · 6 comments · May be fixed by #16836
Assignees
Labels
C-feature Category: feature

Comments

@sundy-li
Copy link
Member

sundy-li commented Nov 7, 2024

Summary

Description for this feature.

@sundy-li sundy-li added the C-feature Category: feature label Nov 7, 2024
@youngsofun
Copy link
Member

load_can_auto_cast_to is much more strict, only used for load, not suitable for here.

we may introduce another help function like #16072
this closed pr was intended to be used in copy only too (replaced by load_can_auto_cast_to later)

but @andylokandy said

理论上 任何类型都可以互转
所以之前没有 can_cast_to ,cast 不过去是值的问题不是类型问题

I think since currently our cast (run_cast) do report error on some (src, dst) pairs.
may be it is ok to add a can_cast_to which is consistent with run_cast to report error earlier when building pipeline,
and check it when constructing TransformCastSchema

@sundy-li
Copy link
Member Author

sundy-li commented Nov 8, 2024

But run_cast will check the cast types and throw

  _ => Err(ErrorCode::BadArguments(format!(
                "unable to cast type `{src_type}` to type `{dest_type}`"
            ))
            .set_span(span)),

@andylokandy
Copy link
Collaborator

andylokandy commented Nov 8, 2024

We can say that a value is not able to be cast to another type, e.g. CAST('a' AS INT), but we can not say that all String can not be cast to Int. The former will throw at evaluation, and the latter will throw at type checking.

@sundy-li
Copy link
Member Author

sundy-li commented Nov 8, 2024

The former will throw at evaluation, and the latter will throw at type checking.

The latter will not throw errors at type checking, let's add it.

It will always return Expr::Cast

fn check_cast<Index: ColumnIndex> ... {

 Ok(Expr::Cast {
            span,
            is_try,
            expr: Box::new(expr),
            dest_type: wrapped_dest_type,
        })
}

@sundy-li sundy-li assigned andylokandy and unassigned youngsofun Nov 8, 2024
@sundy-li
Copy link
Member Author

sundy-li commented Nov 8, 2024

A simple case, the error is thrown during sql pipeline execution.

🐳 :) create table a (t decimal(15,2));
processed in (0.018 sec)


🐳 :) insert into a select * from b;
error: APIError: ResponseError with 1006: fail to auto cast column t (Variant NULL) to column t (Decimal(15, 2) NULL)
unable to cast type `Variant` to type `Decimal(15, 2)`, during run expr: `CAST(t AS Decimal(15, 2) NULL)`

@andylokandy
Copy link
Collaborator

The former will throw at evaluation, and the latter will throw at type checking.

I mean, if we agree that all types can convert between each other, i.e.
We can say that a value is not able to be cast to another type, e.g. CAST('a' AS INT), but we can not say that all String can not be cast to Int., we should not throw at type checking.

@sundy-li sundy-li linked a pull request Dec 4, 2024 that will close this issue
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants