jonahgao commented on code in PR #15183:
URL: https://github.com/apache/datafusion/pull/15183#discussion_r2007825842
##########
datafusion/sql/src/planner.rs:
##########
@@ -754,7 +772,11 @@ pub fn object_name_to_table_reference(
enable_normalization: bool,
) -> Result<TableReference> {
// Use destructure to make it clear no fields on ObjectName are ignored
- let ObjectName(idents) = object_name;
+ let ObjectName(object_name_parts) = object_name;
+ let idents = object_name_parts
+ .into_iter()
+ .map(|object_name_part| object_name_part.as_ident().unwrap().clone())
Review Comment:
Replace unwrap with error.
##########
datafusion/sql/src/select.rs:
##########
@@ -92,6 +93,9 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
planner_context,
)?;
+ let order_by =
+ to_order_by_exprs_with_select(query_order_by,
Some(select_exprs.clone()))?;
Review Comment:
This cloning incurs extra costs. How about passing `select_exprs` by
reference
##########
datafusion/sql/src/planner.rs:
##########
@@ -836,21 +858,31 @@ pub(crate) fn idents_to_table_reference(
pub fn object_name_to_qualifier(
sql_table_name: &ObjectName,
enable_normalization: bool,
-) -> String {
+) -> Result<String> {
let columns = vec!["table_name", "table_schema",
"table_catalog"].into_iter();
let normalizer = IdentNormalizer::new(enable_normalization);
sql_table_name
.0
.iter()
.rev()
.zip(columns)
- .map(|(ident, column_name)| {
- format!(
- r#"{} = '{}'"#,
- column_name,
- normalizer.normalize(ident.clone())
- )
+ .map(|(object_name_part, column_name)| {
+ object_name_part
+ .as_ident()
+ .map(|ident| {
+ format!(
+ r#"{} = '{}'"#,
+ column_name,
+ normalizer.normalize(ident.clone())
+ )
+ })
+ .ok_or_else(|| {
+ DataFusionError::Plan(format!(
Review Comment:
nit: use `plan_datafusion_err`
##########
datafusion/sql/src/statement.rs:
##########
@@ -907,18 +946,24 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
returning,
or,
} => {
- let from =
+ let froms =
from.map(|update_table_from_kind| match
update_table_from_kind {
- UpdateTableFromKind::BeforeSet(from) => from,
- UpdateTableFromKind::AfterSet(from) => from,
+ UpdateTableFromKind::BeforeSet(froms) => froms.clone(),
+ UpdateTableFromKind::AfterSet(froms) => froms.clone(),
});
+ // TODO: support multiple tables in UPDATE SET FROM
+ if froms.clone().is_some_and(|f| f.len() > 1) {
+ println!("---------------------------------------------");
Review Comment:
```suggestion
if froms.as_ref().is_some_and(|f| f.len() > 1) {
```
##########
datafusion/sql/src/relation/join.rs:
##########
@@ -136,7 +136,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
)
} else {
let id = object_names.swap_remove(0);
- Ok(self.ident_normalizer.normalize(id))
+ id.as_ident()
+ .ok_or_else(|| {
+ datafusion_common::DataFusionError::Plan(
Review Comment:
Use `plan_datafusion_err`
##########
datafusion/sql/src/statement.rs:
##########
@@ -907,18 +946,24 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
returning,
or,
} => {
- let from =
+ let froms =
from.map(|update_table_from_kind| match
update_table_from_kind {
- UpdateTableFromKind::BeforeSet(from) => from,
- UpdateTableFromKind::AfterSet(from) => from,
+ UpdateTableFromKind::BeforeSet(froms) => froms.clone(),
+ UpdateTableFromKind::AfterSet(froms) => froms.clone(),
Review Comment:
```suggestion
UpdateTableFromKind::BeforeSet(froms) => froms,
UpdateTableFromKind::AfterSet(froms) => froms,
```
Is it possible to remove these clones?
##########
datafusion/sql/src/statement.rs:
##########
@@ -907,18 +946,24 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
returning,
or,
} => {
- let from =
+ let froms =
from.map(|update_table_from_kind| match
update_table_from_kind {
- UpdateTableFromKind::BeforeSet(from) => from,
- UpdateTableFromKind::AfterSet(from) => from,
+ UpdateTableFromKind::BeforeSet(froms) => froms.clone(),
+ UpdateTableFromKind::AfterSet(froms) => froms.clone(),
});
+ // TODO: support multiple tables in UPDATE SET FROM
+ if froms.clone().is_some_and(|f| f.len() > 1) {
+ println!("---------------------------------------------");
+ plan_err!("Multiple tables in UPDATE SET FROM not yet
supported")?;
+ }
+ let update_from = froms.map(|f| f.first().unwrap().clone());
Review Comment:
```suggestion
let update_from = froms.and_then(|mut f| f.pop());
```
##########
datafusion/sql/src/statement.rs:
##########
@@ -75,7 +76,11 @@ fn object_name_to_string(object_name: &ObjectName) -> String
{
object_name
.0
.iter()
- .map(ident_to_string)
+ .map(|object_name_part| {
+ object_name_part
+ .as_ident()
+ .map_or_else(String::new, ident_to_string)
Review Comment:
It might be better to return an error than to silently use a default value.
--
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
