alamb commented on code in PR #11329:
URL: https://github.com/apache/datafusion/pull/11329#discussion_r1669083208
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -403,22 +403,33 @@ pub async fn from_substrait_rel(
let mut input = LogicalPlanBuilder::from(
from_substrait_rel(ctx, input, extensions).await?,
);
+ let mut names: HashSet<String> = HashSet::new();
let mut exprs: Vec<Expr> = vec![];
for e in &p.expressions {
let x =
from_substrait_rex(ctx, e, input.clone().schema(),
extensions)
.await?;
// if the expression is WindowFunction, wrap in a Window
relation
- // before returning and do not add to list of this
Projection's expression list
- // otherwise, add expression to the Projection's
expression list
- match &*x {
- Expr::WindowFunction(_) => {
- input = input.window(vec![x.as_ref().clone()])?;
- exprs.push(x.as_ref().clone());
- }
- _ => {
- exprs.push(x.as_ref().clone());
- }
+ if let Expr::WindowFunction(_) = x.as_ref() {
+ // Adding the same expression here and in the project
below
+ // works because the project's builder uses
columnize_expr(..)
+ // to transform it into a column reference
+ input = input.window(vec![x.as_ref().clone()])?
+ }
+ // Ensure the expression has a unique display name, so
that project's
+ // validate_unique_names doesn't fail
+ let name = x.display_name()?;
+ let mut new_name = name.clone();
+ let mut i = 0;
+ while names.contains(&new_name) {
+ new_name = format!("{}__temp__{}", name, i);
+ i += 1;
+ }
+ names.insert(new_name.clone());
Review Comment:
You could probably save a clone by putting the `names.insert` call below the
`exprs.push` calls
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -403,22 +403,33 @@ pub async fn from_substrait_rel(
let mut input = LogicalPlanBuilder::from(
from_substrait_rel(ctx, input, extensions).await?,
);
+ let mut names: HashSet<String> = HashSet::new();
let mut exprs: Vec<Expr> = vec![];
for e in &p.expressions {
let x =
from_substrait_rex(ctx, e, input.clone().schema(),
extensions)
.await?;
// if the expression is WindowFunction, wrap in a Window
relation
- // before returning and do not add to list of this
Projection's expression list
- // otherwise, add expression to the Projection's
expression list
- match &*x {
- Expr::WindowFunction(_) => {
- input = input.window(vec![x.as_ref().clone()])?;
- exprs.push(x.as_ref().clone());
- }
- _ => {
- exprs.push(x.as_ref().clone());
- }
+ if let Expr::WindowFunction(_) = x.as_ref() {
+ // Adding the same expression here and in the project
below
+ // works because the project's builder uses
columnize_expr(..)
+ // to transform it into a column reference
+ input = input.window(vec![x.as_ref().clone()])?
+ }
+ // Ensure the expression has a unique display name, so
that project's
+ // validate_unique_names doesn't fail
+ let name = x.display_name()?;
+ let mut new_name = name.clone();
+ let mut i = 0;
+ while names.contains(&new_name) {
+ new_name = format!("{}__temp__{}", name, i);
+ i += 1;
+ }
+ names.insert(new_name.clone());
+ if new_name != name {
+ exprs.push(x.as_ref().clone().alias(new_name.clone()));
+ } else {
+ exprs.push(x.as_ref().clone());
Review Comment:
I was confused at first why this was `as_ref().clone()` was needed
When I investigated, it seems that it is due to the fact that
`from_substrait_rex` returns an `Arc<Expr>` rather than an `Expr` which is
somewhat confusing 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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
