Combine Expr::Wildcard and Wxpr::QualifiedWildcard, add wildcard() expr fn#8105
Combine Expr::Wildcard and Wxpr::QualifiedWildcard, add wildcard() expr fn#8105alamb merged 1 commit intoapache:mainfrom
Expr::Wildcard and Wxpr::QualifiedWildcard, add wildcard() expr fn#8105Conversation
github-actions
Bot
added
sql
| .aggregate(vec![col("b")], vec![count(Wildcard)])? | ||
| .sort(vec![count(Wildcard).sort(true, false)])? | ||
| .aggregate(vec![col("b")], vec![count(wildcard())])? | ||
| .sort(vec![count(wildcard()).sort(true, false)])? |
There was a problem hiding this comment.
this shows the difference in API for users (calling wildcard()), which I think is more consistent with other functions
| } | ||
| Expr::QualifiedWildcard { ref qualifier } => projected_expr | ||
| .extend(expand_qualified_wildcard(qualifier, input_schema, None)?), | ||
| Expr::Wildcard { |
There was a problem hiding this comment.
here is the only place where Wildcard and QualifiedWildcard are treated substantially differently
| })), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
As a bonus, qualified wildcards can now be serialized to/from proto -- I doubt this feature is widely used, but it is nice to avoid the rough edge I think
waynexia
left a comment
waynexia
left a comment
There was a problem hiding this comment.
Makes sense to me. Thanks for taking this 👍
| pub fn wildcard() -> Expr { | ||
| Expr::Wildcard { qualifier: None } | ||
| } |
There was a problem hiding this comment.
Do we need a fn wildcard_with_qualifier() or fn wildcard(qualifier: None)? Though it's not used in our examples.
There was a problem hiding this comment.
I was thinking that it was such a niche usecase that if anyone needed the qualifier they could make it explicitly via Expr::Wildcard{ qualifier: Some(...)}, but that is indeed messier
I would be happy to add qualified_wildcard or similar if others think it would be valuable
|
I plan to leave this PR open for a few days to given anyone who might be interested time to comment. Thank you @waynexia for the review 🙏 |
2 participants
Which issue does this PR close?
Closes #8104
Rationale for this change
Per #8104
As @waynexia points out on #8008 (comment), having
Expr::WildcardandExpr::QualifiedWildcardis confusing as they are both representing the same basic pattern,*Specifically, I think it could make subtle bugs more likely (e.g. using
Expr::Wildcardinstead of usingExpr::QualifiedWildcard)What changes are included in this PR?
Expr::WildcardtoExprWildcard { qualifier: Option<String> }and removeExpr::QualifiedWildcardAre these changes tested?
Are there any user-facing changes?
Yes, users can't use
Expr::Wildcardanymore -- they either have to useExpr::Wildcard { qualifier: None}orwildcard()