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 Cast name for display #10276

Closed
wants to merge 9 commits into from
Closed

Better Cast name for display #10276

wants to merge 9 commits into from

Conversation

yyy1000
Copy link
Contributor

@yyy1000 yyy1000 commented Apr 29, 2024

Which issue does this PR close?

Closes #10274 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Apr 29, 2024
@yyy1000 yyy1000 marked this pull request as draft April 29, 2024 01:35
@github-actions github-actions bot added the optimizer Optimizer rules label Apr 29, 2024
@github-actions github-actions bot added the core Core DataFusion crate label Apr 29, 2024
@yyy1000 yyy1000 marked this pull request as ready for review April 29, 2024 05:20
@yyy1000
Copy link
Contributor Author

yyy1000 commented Apr 29, 2024

I encountered a problem for type_coercion and need help🥲
Here TypeCoercion would rewrite an Expr thus brings a 'Cast', and the name for Cast would not just be the expr name in the Cast in this PR, thus rewrite_preserving_name will introduce an Alias, which is not expected.

let mut expr_rewrite = TypeCoercionRewriter {
schema: Arc::new(schema),
};
let new_expr = plan
.expressions()
.into_iter()
.map(|expr| {
// ensure aggregate names don't change:
// https://github.com/apache/datafusion/issues/3555
rewrite_preserving_name(expr, &mut expr_rewrite)
})
.collect::<Result<Vec<_>>>()?;
plan.with_new_exprs(new_expr, new_inputs)

@jayzhan211
Copy link
Contributor

jayzhan211 commented Apr 29, 2024

I think we should change the display_name not create_name, as the comment said, name for casting should preserve

See #3727 for why preserving name

@github-actions github-actions bot removed the core Core DataFusion crate label Apr 29, 2024
@github-actions github-actions bot added the sql SQL Planner label Apr 29, 2024
@alamb alamb marked this pull request as draft April 29, 2024 19:41
@alamb
Copy link
Contributor

alamb commented Apr 29, 2024

Looks like this one has some test failures to review before it is ready for review

@yyy1000
Copy link
Contributor Author

yyy1000 commented Apr 30, 2024

I encountered an issue: the plan's schema would change during rewrite stage.👀
In

explain verbose SELECT CAST(2 AS INTEGER);

+----------------------------------------------------------------------------+--------------------------------------------------------------------------------------+
| plan_type                                                                  | plan                                                                                 |
+----------------------------------------------------------------------------+--------------------------------------------------------------------------------------+
| initial_logical_plan                                                       | Projection: CAST(Int64(2) AS Int32)                                                  |
|                                                                            |   EmptyRelation                                                                      |
| logical_plan after apply_function_rewrites                                 | SAME TEXT AS ABOVE                                                                   |
| logical_plan after inline_table_scan                                       | SAME TEXT AS ABOVE                                                                   |
| logical_plan after type_coercion                                           | SAME TEXT AS ABOVE                                                                   |
| logical_plan after count_wildcard_rule                                     | SAME TEXT AS ABOVE                                                                   |
| analyzed_logical_plan                                                      | SAME TEXT AS ABOVE                                                                   |
| logical_plan after eliminate_nested_union                                  | SAME TEXT AS ABOVE                                                                   |
| logical_plan after simplify_expressions                                    | Projection: Int32(2) AS Int64(2)                                                     |
|                                                                            |   EmptyRelation                                                                      |
| logical_plan after unwrap_cast_in_comparison                               | SAME TEXT AS ABOVE                                                                   |
| logical_plan after replace_distinct_aggregate                              | SAME TEXT AS ABOVE                                                                   |
| logical_plan after eliminate_join                                          | SAME TEXT AS ABOVE                                                                   |
| logical_plan after decorrelate_predicate_subquery                          | SAME TEXT AS ABOVE                                                                   |
| logical_plan after scalar_subquery_to_join                                 | SAME TEXT AS ABOVE                                                                   |
| logical_plan after extract_equijoin_predicate                              | SAME TEXT AS ABOVE                                                                   |
| logical_plan after simplify_expressions                                    | SAME TEXT AS ABOVE                                                                   |
| logical_plan after rewrite_disjunctive_predicate                           | SAME TEXT AS ABOVE                                                                   |
| logical_plan after eliminate_duplicated_expr                               | SAME TEXT AS ABOVE                                                                   |
| logical_plan after eliminate_filter                                        | SAME TEXT AS ABOVE                                                                   |
| logical_plan after eliminate_cross_join                                    | SAME TEXT AS ABOVE                                                                   |
| logical_plan after Optimizer rule 'common_sub_expression_eliminate' failed | Schema error: No field named "CAST(Int64(2) AS Int32)". Valid fields are "Int64(2)". |
+----------------------------------------------------------------------------+--------------------------------------------------------------------------------------+
20 row(s) fetched. 
Elapsed 0.008 seconds.

After simplify_expressions, ConstEvaluator will rewrite the Cast to an Alias to ScalarValue, and the name for the Alias is different from the Field name in the original schema (Because in the past the Cast's name is just the expr, which is the same as the Alias's name, but now it's not.

I will dive into related code but it may take some time. :) I'm not so familiar with related code base but I will try to learn it.

Copy link

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label Jun 29, 2024
@github-actions github-actions bot closed this Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner Stale PR has not had any activity for some time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better name display for CAST
3 participants