-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Qualified wildcard #2012
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
Qualified wildcard #2012
Conversation
|
Would you please find time to review this pr? |
|
Thanks @doki23, will take a look tomorrow :) |
| Expr::Wildcard => Err(DataFusionError::Internal( | ||
| "Create name does not support wildcard".to_string(), | ||
| )), | ||
| Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can create a name for QualifiedWildcard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree including the name of the qualifier in the message might be helpful
| Expr::Wildcard => Err(DataFusionError::Internal( | ||
| "Create physical name does not support wildcard".to_string(), | ||
| )), | ||
| Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking very nice -- thank you @doki23
| Expr::Wildcard => Err(DataFusionError::Internal( | ||
| "Create name does not support wildcard".to_string(), | ||
| )), | ||
| Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree including the name of the qualifier in the message might be helpful
|
Upon further thought, I wonder if it would make sense to expand out all qualified wildcards in the Though I suppose if we did that then we couldn't support wild cards in the DataFrame API 🤔 |
|
I think it's ready for review |
We should support wildcard in any kinds of api, including DataFrame. |
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good to me -- thanks @doki23 !
| Ok(_) => panic!("unexpected OK"), | ||
| Err(err) => assert_eq!( | ||
| err.to_string(), | ||
| "Error during planning: Invalid qualifier agg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| }, | ||
| /// Represents a reference to all fields in a schema. | ||
| Wildcard, | ||
| /// Represents a reference to all fields in a specific schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally have a slight preference for a single Expr::Wildcard with optional qualifier
Wildcard { qualifier: Option<String>}but that is a personal preference and there is nothing wrong with this approach
xudong963
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks again @doki23 |
|
Thanks @xudong963 @alamb ! ❤️ |
Which issue does this PR close?
Closes #1994.
What changes are included in this PR?
Add enum Expr::QualifiedWildcard
Add function: expand_qualified_wildcard
Add some tests