fix: preserve qualifiers when rewriting expressions

[jonahgao]

Which issue does this PR close?

A more general fix for #12183

Rationale for this change

The two expressions in the following code have the same schema_names, so the NamePreserver will not take effect when expr1 is rewritten to expr2. But they will produce different fields, which will result in schema changes.

Therefore, the NamePreserver needs to preserve both the qualifier and the schema name to ensure that the rewritten expression has the same to_field() result as before.

   #[test]
    fn test_expr_schema_name() {
        let schema = Schema::new(vec![
            Field::new("a", DataType::Int32, true),
            Field::new("test.a", DataType::Utf8, true),
        ]);
        let dfschema = DFSchema::try_from_qualified_schema("test", &schema).unwrap();
        let expr1 = Expr::Column(Column::new_unqualified("test.a"));
        let expr2 = Expr::Column(Column::new(Some("test"), "a"));

        assert_eq!(
            expr1.schema_name().to_string(),
            expr2.schema_name().to_string()
        );

        let field1 = expr1.to_field(&dfschema).unwrap();
        let field2 = expr2.to_field(&dfschema).unwrap();

        // field1: qualifier: None, name: "test.a"
        println!(
            "field1: qualifier: {:?}, name: {:?}",
            field1.0,
            field1.1.name()
        );
        // field2: qualifier: Some(Bare { table: "test" }), name: "a"
        println!(
            "field2: qualifier: {:?}, name: {:?}",
            field2.0,
            field2.1.name()
        );
    }

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

[jonahgao]

The return value no longer needs Result, but removing it breaks many optimization rules. Maybe we can handle it in the next PR.

[alamb]

Looks like a very nice fix to me -- thank you @jonahgao

cc @JasonLi-cn

[alamb]

I would personally suggest creating an enum to make this more explicit (rather than two level of options)-- perhaps something like

pub enum SavedName {
  ///  name is not preserved
  None, 
  /// qualified name is preserved
  Saved {
    relation: QualifiedName,
    name: String,
  }
}

[jonahgao]

Done.

[alamb]

I seems like we could potentially also remove Expr::alias_if_changed which doesn't properly account for qualifiers 🤔

The only other use of it seems to be in

datafusion/datafusion/expr/src/expr_rewriter/mod.rs

Line 287 in e603185

pub fn rewrite_preserving_name<R>(expr: Expr, rewriter: &mut R) -> Result<Expr>

which itself is only used in tests 🤔

[jonahgao]

alias_if_changed is being used by substrait. Maybe we can review this later.

rewrite_preserving_name has been removed.

[jonahgao]

Tests use NamePreserver instead of rewrite_preserving_name.

[jonahgao]

Thanks @JasonLi-cn @alamb @Dandandan