Skip to content
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

Remove any aliases in Filter::try_new rather than erroring #11307

Merged
merged 4 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion datafusion/core/tests/user_defined/user_defined_sql_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ use datafusion::execution::FunctionRegistry;
use datafusion::logical_expr::Operator;
use datafusion::prelude::*;
use datafusion::sql::sqlparser::ast::BinaryOperator;
use datafusion_common::ScalarValue;
use datafusion_expr::expr::Alias;
use datafusion_expr::planner::{PlannerResult, RawBinaryExpr, UserDefinedSQLPlanner};
use datafusion_expr::BinaryExpr;

Expand All @@ -50,13 +52,22 @@ impl UserDefinedSQLPlanner for MyCustomPlanner {
op: Operator::Plus,
})))
}
BinaryOperator::Question => {
Ok(PlannerResult::Planned(Expr::Alias(Alias::new(
Expr::Literal(ScalarValue::Boolean(Some(true))),
None::<&str>,
format!("{} ? {}", expr.left, expr.right),
))))
}
_ => Ok(PlannerResult::Original(expr)),
}
}
}

async fn plan_and_collect(sql: &str) -> Result<Vec<RecordBatch>> {
let mut ctx = SessionContext::new();
let config =
SessionConfig::new().set_str("datafusion.sql_parser.dialect", "postgres");
let mut ctx = SessionContext::new_with_config(config);
ctx.register_user_defined_sql_planner(Arc::new(MyCustomPlanner))?;
ctx.sql(sql).await?.collect().await
}
Expand Down Expand Up @@ -86,3 +97,27 @@ async fn test_custom_operators_long_arrow() {
];
assert_batches_eq!(&expected, &actual);
}

#[tokio::test]
async fn test_question_select() {
let actual = plan_and_collect("select a ? 2 from (select 1 as a);")
.await
.unwrap();
let expected = [
"+--------------+",
"| a ? Int64(2) |",
"+--------------+",
"| true |",
"+--------------+",
];
assert_batches_eq!(&expected, &actual);
}

#[tokio::test]
async fn test_question_filter() {
let actual = plan_and_collect("select a from (select 1 as a) where a ? 2;")
.await
.unwrap();
let expected = ["+---+", "| a |", "+---+", "| 1 |", "+---+"];
assert_batches_eq!(&expected, &actual);
}
16 changes: 5 additions & 11 deletions datafusion/expr/src/logical_plan/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use std::sync::Arc;
use super::dml::CopyTo;
use super::DdlStatement;
use crate::builder::{change_redundant_column, unnest_with_options};
use crate::expr::{Alias, Placeholder, Sort as SortExpr, WindowFunction};
use crate::expr::{Placeholder, Sort as SortExpr, WindowFunction};
use crate::expr_rewriter::{create_col_from_scalar_expr, normalize_cols};
use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor};
use crate::logical_plan::extension::UserDefinedLogicalNode;
Expand Down Expand Up @@ -2130,16 +2130,10 @@ impl Filter {
}
}

// filter predicates should not be aliased
if let Expr::Alias(Alias { expr, name, .. }) = predicate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused about this check; it is not aligned with unalias_nested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a very old check and substantially predates unalias_nested so it may have become unaligned over time

return plan_err!(
"Attempted to create Filter predicate with \
expression `{expr}` aliased as '{name}'. Filter predicates should not be \
aliased."
);
}

Ok(Self { predicate, input })
Ok(Self {
predicate: predicate.unalias(),
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
input,
})
}

/// Is this filter guaranteed to return 0 or 1 row in a given instantiation?
Expand Down