alamb commented on code in PR #12341:
URL: https://github.com/apache/datafusion/pull/12341#discussion_r1746135863
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -336,12 +338,14 @@ impl NamePreserver {
}
impl SavedName {
- /// Ensures the name of the rewritten expression is preserved
+ /// Ensures the qualified name of the rewritten expression is preserved
pub fn restore(self, expr: Expr) -> Result<Expr> {
let Self(original_name) = self;
match original_name {
- Some(name) => expr.alias_if_changed(name),
Review Comment:
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
https://github.com/apache/datafusion/blob/e60318553d6ac36b2d07466acebef861fde2936e/datafusion/expr/src/expr_rewriter/mod.rs#L287
which itself is only used in tests 🤔
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -303,9 +303,11 @@ pub struct NamePreserver {
use_alias: bool,
}
+type QualifiedName = (Option<TableReference>, String);
Review Comment:
I would personally suggest creating an enum to make this more explicit
(rather than two level of options)-- perhaps something like
```rust
pub enum SavedName {
/// name is not preserved
None,
/// qualified name is preserved
Saved {
relation: QualifiedName,
name: String,
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
