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

Better name display for CAST #10274

Open
yyy1000 opened this issue Apr 28, 2024 · 10 comments
Open

Better name display for CAST #10274

yyy1000 opened this issue Apr 28, 2024 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@yyy1000
Copy link
Contributor

yyy1000 commented Apr 28, 2024

Describe the bug

The type name for CAST is not correct when using datafusion-cli

To Reproduce

DataFusion CLI v37.1.0

SELECT CAST(2 AS INTEGER);
+----------+
| Int64(2) |
+----------+
| 2 |
+----------+
1 row(s) fetched.
Elapsed 0.027 seconds.

SELECT CAST(2 AS DOUBLE);
+----------+
| Int64(2) |
+----------+
| 2.0 |
+----------+
1 row(s) fetched.
Elapsed 0.005 seconds.

SELECT arrow_typeof(CAST(2 AS DOUBLE));
+------------------------+
| arrow_typeof(Int64(2)) |
+------------------------+
| Float64 |
+------------------------+
1 row(s) fetched.
Elapsed 0.008 seconds.

Expected behavior

No response

Additional context

I'm finding what's wrong with it. 🤔

@yyy1000 yyy1000 added the bug Something isn't working label Apr 28, 2024
@jayzhan211
Copy link
Contributor

jayzhan211 commented Apr 29, 2024

The result seems correct to me.

+----------+
| Int64(2) |
+----------+
| 2.0 |
+----------+

i64(2) is the name, not the type.

query RT
SELECT CAST(2 AS DOUBLE), arrow_typeof(CAST(2 AS DOUBLE));
----
2 Float64

@yyy1000
Copy link
Contributor Author

yyy1000 commented Apr 29, 2024

i64(2) is the name, not the type.

Yes, I realized that. I was thinking whether we should use a better name to display, for example CAST(2 AS INTEGER) in the case but not Int64(2) , like it in duckdb. What do you think? @jayzhan211

SELECT CAST(2 as DOUBLE);
┌───────────────────┐
│ CAST(2 AS DOUBLE) │
│      double       │
├───────────────────┤
│               2.0 │
└───────────────────┘

@jayzhan211
Copy link
Contributor

jayzhan211 commented Apr 29, 2024

Strongly agree with a better name!

@yyy1000 yyy1000 changed the title Incorrect type display for CAST Incorrect name display for CAST Apr 29, 2024
@yyy1000 yyy1000 changed the title Incorrect name display for CAST Better name display for CAST Apr 29, 2024
@alamb
Copy link
Contributor

alamb commented Apr 29, 2024

Yes, I realized that. I was thinking whether we should use a better name to display, for example CAST(2 AS INTEGER) in the case but not Int64(2) , like it in duckdb. What do you think? @jayzhan211

I agree formatting Int64(2) as 2 would certainly be nicer

I am not sure about removing the CAST

@jayzhan211
Copy link
Contributor

Yes, I realized that. I was thinking whether we should use a better name to display, for example CAST(2 AS INTEGER) in the case but not Int64(2) , like it in duckdb. What do you think? @jayzhan211

I agree formatting Int64(2) as 2 would certainly be nicer

I am not sure about removing the CAST

I think you are mistaken

Current display is like, which is unclear why i64 type has a result like float, because we do not show the casting

+----------+
| Int64(2) |
+----------+
| 2.0 |
+----------+

Expect display is something like

+----------+
| Cast(Int64(2) as Float)|
+----------+
| 2.0 |
+----------+

@alamb
Copy link
Contributor

alamb commented Apr 30, 2024

Expect display is something like

I agree @jayzhan211 -- sorry for misreading this. I agree that If the user ran

select CAST(2 AS Float)`

seeing this makes sense

+----------+
| Cast(Int64(2) as Float)|
+----------+
| 2.0 |
+----------+

However, if the user wrote something like this

select col = 2

I would expect the result to be something like (even if col was a float column and coercsion applied casts)

+----------+
| col = Int64(2)|
+----------+
| 2.0 |
+----------+

@jayzhan211
Copy link
Contributor

Expect display is something like

I agree @jayzhan211 -- sorry for misreading this. I agree that If the user ran

select CAST(2 AS Float)`

seeing this makes sense

+----------+
| Cast(Int64(2) as Float)|
+----------+
| 2.0 |
+----------+

However, if the user wrote something like this

select col = 2

I would expect the result to be something like (even if col was a float column and coercsion applied casts)

+----------+
| col = Int64(2)|
+----------+
| 2.0 |
+----------+

It looks good to me because we can infer the type from col.

@jayzhan211
Copy link
Contributor

Note: I would like to know if it is possible to have one single function name for logical and physical expression. It is also quite related to the issue here.

@jayzhan211
Copy link
Contributor

@yyy1000 Are you still working on this?

@yyy1000
Copy link
Contributor Author

yyy1000 commented Jul 12, 2024

@yyy1000 Are you still working on this?

No, currently having my internship so I don't have buffer on this. 🥲

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.

3 participants