fix: from_plan shouldn't use original schema #6595

jackwener · 2023-06-08T05:33:20Z

Which issue does this PR close?

part of #6596.
Closes #6613

Rationale for this change

What changes are included in this PR?

In original code, project call from_plan, it will keep original schema even if expression are different.
It's wrong! Because different expression will have different schema. So I correct it.

Are these changes tested?

Are there any user-facing changes?

jackwener · 2023-06-08T08:34:23Z

datafusion/expr/src/utils.rs

-        LogicalPlan::Projection(Projection { schema, .. }) => {
-            Ok(LogicalPlan::Projection(Projection::try_new_with_schema(
-                expr.to_vec(),
-                Arc::new(inputs[0].clone()),
-                schema.clone(),
-            )?))
-        }


Here exist a bug, we shouldn't use original schema, because it can be changed.
Original code hidden some BUG.

alamb

Thank you @jackwener -- this PR is definitely heading in the right direction. Fixing the projection schema is definitely exposing a bunch of bugs.

There are a few things that don't look right in the PR that I commented on.

Thanks again for trying to make things better. It is really apprecaited

alamb · 2023-06-11T11:00:58Z

datafusion/common/src/dfschema.rs

@@ -384,8 +384,7 @@ impl DFSchema {
        let self_fields = self.fields().iter();
        let other_fields = other.fields().iter();
        self_fields.zip(other_fields).all(|(f1, f2)| {
-            f1.qualifier() == f2.qualifier()
-                && f1.name() == f2.name()
+            f1.qualified_name() == f2.qualified_name()


I am not sure if it matters, but I believe that qualified_name creates a new String where the previous version avoids that allocation.

About this, I have a new idea on weekend. we may need handle alias schema() to specify the schema.

Because alias('t1.a'), field is qualifier: none, name: t1.a, we hope field is qualifier: t1, name: a.

I will do it in the future.

related issue #6681

datafusion/core/tests/sql/group_by.rs

alamb · 2023-06-11T12:15:22Z

datafusion/core/tests/sqllogictests/test_files/array.slt

@@ -110,10 +111,10 @@ select array_position(['h', 'e', 'l', 'l', 'o'], 'l', 4), array_position([1, 2,
 4 5 2

 # array_positions scalar function
-query III
+query error DataFusion error: SQL error: ParserError\("Expected an SQL statement, found: caused"\)


these errors look not quite right -- I think there is something wrong with sqllogictest --complete with multi-line errors 🤔

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

alamb

Thank you @jackwener this looks like a nice improvement to me

However, it does seem to introduce a regression in some of the array functions recently introduced by @izveigor . The #6596 (comment) comment suggests to me we need to support NULL in array, so it is a known issue but it might be good to get @izveigor 's opinion

comphead · 2023-06-13T15:54:24Z

datafusion/core/tests/sql/expr.rs

@@ -512,15 +512,22 @@ async fn test_regex_expressions() -> Result<()> {

 #[tokio::test]
 async fn test_cast_expressions() -> Result<()> {
+    test_expression!("CAST('0' AS INT)", "0");


can we move it to slt?

Maybe a good follow on PR

Seems like the hope to was to move it to slt as part of #6210 but wasn't completed 🤔

izveigor · 2023-06-13T18:53:38Z

I created PR about the 'nulls' problem: #6662. So I think it can solve the regression.

jackwener · 2023-06-14T14:15:50Z

I prepare to merge this PR in tomorrow unless there are other comments

I will continue doing more job.

* revert: from_plan keep same schema Project in #6595 * revert: from_plan keep same schema Agg/Window in #6820 * revert type coercion * add comment

* revert: from_plan keep same schema Project in apache#6595 * revert: from_plan keep same schema Agg/Window in apache#6820 * revert type coercion * add comment

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* revert array.slt that changed by #6595 Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * add test for to string Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * first draft Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * cleanup Signed-off-by: jayzhan211 <jayzhan211@gmail.com> --------- Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

github-actions bot added the logical-expr Logical plan and expressions label Jun 8, 2023

jackwener mentioned this pull request Jun 8, 2023

simplify_expressions return expression with wrong type. #6596

Open

github-actions bot added core Core DataFusion crate optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jun 8, 2023

jackwener changed the title ~~expression return wrong expression~~ fix: simplify expression sometimes need to convert type. Jun 8, 2023

jackwener commented Jun 8, 2023

View reviewed changes

jackwener changed the title ~~fix: simplify expression sometimes need to convert type.~~ fix: from_plan shouldn't use original schema & simplify expression need to convert type. Jun 8, 2023

jackwener force-pushed the expr branch 2 times, most recently from bd31450 to 0276537 Compare June 10, 2023 06:04

jackwener marked this pull request as ready for review June 10, 2023 06:04

alamb reviewed Jun 11, 2023

View reviewed changes

jackwener marked this pull request as draft June 12, 2023 09:07

jackwener force-pushed the expr branch from 0276537 to aba14ef Compare June 13, 2023 06:54

jackwener changed the title ~~fix: from_plan shouldn't use original schema & simplify expression need to convert type.~~ fix: from_plan shouldn't use original schema Jun 13, 2023

fix: from_plan shouldn't use original schema

c2e4ea5

jackwener force-pushed the expr branch from aba14ef to c2e4ea5 Compare June 13, 2023 07:10

github-actions bot removed the optimizer Optimizer rules label Jun 13, 2023

jackwener marked this pull request as ready for review June 13, 2023 08:05

jackwener requested a review from alamb June 13, 2023 08:05

alamb approved these changes Jun 13, 2023

View reviewed changes

comphead reviewed Jun 13, 2023

View reviewed changes

jackwener merged commit 36123ee into apache:main Jun 15, 2023

jackwener deleted the expr branch June 15, 2023 09:58

jackwener mentioned this pull request Jun 15, 2023

RFC: add a field for Expr::Alias #6681

Closed

izveigor mentioned this pull request Jun 21, 2023

feat: support NULL in array functions #6662

Merged

jackwener added a commit to jackwener/arrow-datafusion that referenced this pull request Jul 3, 2023

revert apache#6595

600ad76

jackwener mentioned this pull request Jul 3, 2023

Schema error: No field named mytable."A". when table is named "MyTable" #6790

Closed

jackwener added a commit to jackwener/arrow-datafusion that referenced this pull request Jul 3, 2023

revert apache#6595

8ba8f85

jackwener added a commit to jackwener/arrow-datafusion that referenced this pull request Jul 3, 2023

revert: from_plan keep same schema Project in apache#6595

a08ea21

jackwener added a commit to jackwener/arrow-datafusion that referenced this pull request Jul 3, 2023

revert: from_plan keep same schema Project in apache#6595

e44eb0e

jackwener added a commit that referenced this pull request Jul 4, 2023

revert #6595 #6820 (#6827)

137bf81

* revert: from_plan keep same schema Project in #6595 * revert: from_plan keep same schema Agg/Window in #6820 * revert type coercion * add comment

jackwener mentioned this pull request Jul 6, 2023

fix: from_plan generate incorrect schema when it is Aggregate #6862

Closed

maxburke mentioned this pull request Jul 9, 2023

Fix for issue #6595 has broken existing working queries ("Schema error: No field named ...") #6897

Closed

jayzhan211 added a commit to jayzhan211/datafusion that referenced this pull request Jul 13, 2023

revert array.slt that changed by apache#6595

f841f98

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 mentioned this pull request Jul 13, 2023

Column support for array_to_string #6940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: from_plan shouldn't use original schema #6595

fix: from_plan shouldn't use original schema #6595

jackwener commented Jun 8, 2023 •

edited

Loading

jackwener Jun 8, 2023

alamb left a comment

alamb Jun 11, 2023

jackwener Jun 11, 2023 •

edited

Loading

jackwener Jun 15, 2023

alamb Jun 11, 2023

alamb left a comment

comphead Jun 13, 2023

alamb Jun 13, 2023

izveigor commented Jun 13, 2023

jackwener commented Jun 14, 2023 •

edited

Loading

fix: from_plan shouldn't use original schema #6595

fix: from_plan shouldn't use original schema #6595

Conversation

jackwener commented Jun 8, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jackwener Jun 8, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 11, 2023

Choose a reason for hiding this comment

jackwener Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

jackwener Jun 15, 2023

Choose a reason for hiding this comment

alamb Jun 11, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

comphead Jun 13, 2023

Choose a reason for hiding this comment

alamb Jun 13, 2023

Choose a reason for hiding this comment

izveigor commented Jun 13, 2023

jackwener commented Jun 14, 2023 • edited Loading

jackwener commented Jun 8, 2023 •

edited

Loading

jackwener Jun 11, 2023 •

edited

Loading

jackwener commented Jun 14, 2023 •

edited

Loading