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

Move sql_compound_identifier_to_expr to ExprPlanner #11487

Merged
merged 20 commits into from
Jul 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 8 additions & 0 deletions datafusion/expr/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ pub trait ExprPlanner: Send + Sync {
fn plan_overlay(&self, args: Vec<Expr>) -> Result<PlannerResult<Vec<Expr>>> {
Ok(PlannerResult::Original(args))
}

/// Plans expression to get an index field. Indexed field is only valid for `List`, `Struct`, `Map` or `Null`.
/// eg `expr["name"]`
///
/// Returns origin expression arguments if not possible
fn plan_get_field(&self, args: Vec<Expr>) -> Result<PlannerResult<Vec<Expr>>> {
dharanad marked this conversation as resolved.
Show resolved Hide resolved
Ok(PlannerResult::Original(args))
}
}

/// An operator with two arguments to plan
Expand Down
1 change: 0 additions & 1 deletion datafusion/functions/src/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ pub fn functions() -> Vec<Arc<ScalarUDF>> {
nvl2(),
arrow_typeof(),
named_struct(),
get_field(),
coalesce(),
make_map(),
map(),
Expand Down
6 changes: 6 additions & 0 deletions datafusion/functions/src/core/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,10 @@ impl ExprPlanner for CoreFunctionPlanner {
ScalarFunction::new_udf(crate::string::overlay(), args),
)))
}

fn plan_get_field(&self, args: Vec<Expr>) -> Result<PlannerResult<Vec<Expr>>> {
Ok(PlannerResult::Planned(Expr::ScalarFunction(
ScalarFunction::new_udf(crate::core::get_field(), args),
)))
}
}
28 changes: 16 additions & 12 deletions datafusion/sql/src/expr/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@
// specific language governing permissions and limitations
// under the License.

use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
use arrow_schema::Field;
use sqlparser::ast::{Expr as SQLExpr, Ident};

use datafusion_common::{
internal_err, not_impl_err, plan_datafusion_err, Column, DFSchema, DataFusionError,
Result, ScalarValue, TableReference,
};
use datafusion_expr::{expr::ScalarFunction, lit, Case, Expr};
use sqlparser::ast::{Expr as SQLExpr, Ident};
use datafusion_expr::planner::PlannerResult;
use datafusion_expr::{lit, Case, Expr};

use crate::planner::{ContextProvider, PlannerContext, SqlToRel};

impl<'a, S: ContextProvider> SqlToRel<'a, S> {
pub(super) fn sql_identifier_to_expr(
Expand Down Expand Up @@ -135,16 +138,17 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let nested_name = nested_names[0].to_string();

let col = Expr::Column(Column::from((qualifier, field)));
if let Some(udf) =
self.context_provider.get_function_meta("get_field")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder could we plan the whole compound identifier, the benefit of plan_* function is able to customize the operator (in this case is compound identifier i.e. a.b.c) and expressions we get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally i wanted to do that same, but this function uses IdentNormalizer field of SqlToRel struct. I am just thinking a way around & wanted to discuss the same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the result of self.normalizer.normalize() as the planner arguments, similar to the spirit of others function that we take the result of sql_expr_to_logical_expr as the planner arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this revision ? Do you think we aligned here ? There is still more work to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fn sql_compound_identifier_to_expr does many things, handles the logic for variable, compund indentifier & compund indentifier in outer query. In this change we are only supporting compund indentifier & not adding support compund indentifier in outer query.

{
Ok(Expr::ScalarFunction(ScalarFunction::new_udf(
udf,
vec![col, lit(ScalarValue::from(nested_name))],
)))
} else {
internal_err!("get_field not found")
let mut get_field_args =
vec![col, lit(ScalarValue::from(nested_name))];
for planner in self.planners.iter() {
match planner.plan_get_field(get_field_args)? {
PlannerResult::Planned(expr) => return Ok(expr),
PlannerResult::Original(args) => get_field_args = args,
}
}
not_impl_err!(
"GetField not supported by ExprPlanner: {get_field_args:?}"
)
}
// found matching field with no spare identifier(s)
Some((field, qualifier, _nested_names)) => {
dharanad marked this conversation as resolved.
Show resolved Hide resolved
Expand Down