You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "waynexia (via GitHub)" <gi...@apache.org> on 2023/10/07 08:54:23 UTC
[I] proposal: deprecate `Expr::Wildcard` [arrow-datafusion]
waynexia opened a new issue, #7765:
URL: https://github.com/apache/arrow-datafusion/issues/7765
### Is your feature request related to a problem or challenge?
`Expr::Wildcard` and `Expr::QualifiedWildcard` are expressions that reference all columns. But it seems redundant that we don't need a special expr type to do that. This issue proposes to remove these two expr kinds.
### Describe the solution you'd like
Wildcard (`*`) can be expanded to concrete column lists when it appears. This manner seems viable in the three most common use cases I can come up with:
- (1) generate plan from SQL AST. Some scenarios already acted like this. E.g., `select * from table` will generate a projection with all fields, rather than a wildcard expr.
- (2) from `LogicalPlanBuilder` and (3) from `DataFrame`. In some aspects, these two entrances are the same. And both are strong typed (or strong schema-ed), which allows us to expand the wildcard immediately using the schema from current stage.
Besides this, `Expr::Wildcard` is not properly handled in the codebase, because it's not a "first class" expr. Take some functions as examples:
- `expr_to_columns`: from the correctness aspect, it should also count columns referenced by `Expr::Wildcard` and `Expr::QualifiedWildcard`.
- `create_physical_name`: this function requires all the wildcards to be expanded before calling it.
And I find another issue that related to `Wildcard`: https://github.com/apache/arrow-datafusion/issues/5473. The [solution](https://github.com/apache/arrow-datafusion/pull/5627) is to add an optimizer rule that expands all wildcards. This proposal is going to do something similar but in a more eager way.
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
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: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Remove `Wildcard` from `Expr` [datafusion]
Posted by "rkrishn7 (via GitHub)" <gi...@apache.org>.
rkrishn7 commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2654922651
From a user perspective, my opinion is that representing wildcard with `None` is too implicit. As @alamb mentioned, having an `expr_fn` helper makes this a bit better, but still not great IMO.
> As long as we have alternative representation of wildcard (i.e. None in projection expressions)
How would we differentiate qualified vs. unqualified wildcards with `None`?
Just to think about alternatives, what if we kept `Expr::Wildcard` around and guarantee that it is expanded in any `LogicalPlan`s built via `LogicalPlanBuilder` (related to #14618)?.
We could also think about a type safe way to represent this, but I think that would come with significant breaking changes.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Remove `Wildcard` from `Expr` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2692710667
Here is a PR that marks Expr::Wildcard deprecated (thanks @linhr 🙏 )
- https://github.com/apache/datafusion/pull/14959
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2648105811
BTW it's clear some expressions are mere hacks, not real expressions. It's enough to look at `expr.get_type` impl and both `Wildcard` and `GroupingSet` stand out as such:
https://github.com/apache/datafusion/blob/68e372f2952bae884590f0589f23e28fb8bc3eaf/datafusion/expr/src/expr_schema.rs#L214-L218
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2649611265
I don't think we use wildcard for count in datafusion, `COUNT_STAR_EXPANSION` is used instead which is `count(1)`. As long as we have alternative representation of wildcard (i.e. None in projection expressions) then removing it makes sense to me.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2648305668
> `count(*)` is a special syntax that needs to be handled explicitly -- perhaps even at the parsing phase.
> It doesn't constitute a reason to keep wildcard as an expression.
I agree
As long as we also have some way to represent the same thing in the expr_fn API I don't see any reason that we need to have `Expr::Wildcard`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on issue #7765:
URL: https://github.com/apache/datafusion/issues/7765#issuecomment-2648101769
`count(*)` is a special syntax that needs to be handled explicitly -- perhaps even at the parsing phase.
It doesn't constitute a reason to keep wildcard as an expression.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [arrow-datafusion]
Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on issue #7765:
URL: https://github.com/apache/arrow-datafusion/issues/7765#issuecomment-1751686327
>Concretely, what would be the equivalent to the following?
That's the point 👍 For `select * from table` we can simply omit the projection. But this doesn't work for `count(*)`. I was thinking about letting "nothing" to represent the wildcard, similar to [pandas](https://pandas.pydata.org/docs/reference/api/pandas.core.groupby.DataFrameGroupBy.count.html) or [substrait](https://substrait.io/extensions/functions_aggregate_generic/#count_1). But this is a little bad as we have to use `Option<Expr>` instead of overload `count()`. An alternative is to add a specific `count()` variant for `count(*)`, like `count_all()`. But whatever this API looks like, we must change the outer usage because the schema is unavailable for an independent `count()` method. `Wildcard` helps to postpone the expanding, and we have to do it immediately if it's removed.
The lucky thing is `Expr::Wildcard` is not that generic. So we only handle a few cases about it (or maybe the `count(*)` is the only case? I have this feeling but I am not sure about it).
>BTW maybe we could improve the situation by adding more documentation to the Expr::Wildcard
Makes sense, I'll add it first.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] proposal: deprecate `Expr::Wildcard` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #7765:
URL: https://github.com/apache/arrow-datafusion/issues/7765#issuecomment-1751681178
@waynexia -- I agree with your points about `Expr::Wildcard` being confusing. I didn't quite understand your proposal to `change the public interface that may generate wildcard`, specifically how someone would express "select all the columns" when building expressions programmatically 🤔
Concretely, what would be the equivalent to the following?
```rust
ctx.table("alltypes_plain")
.await?
.select(vec![count(Expr::Wildcard)])
```
BTW we
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org