alamb commented on code in PR #4530:
URL: https://github.com/apache/arrow-datafusion/pull/4530#discussion_r1041383281
##########
datafusion/common/src/dfschema.rs:
##########
@@ -206,7 +206,7 @@ impl DFSchema {
(Some(qq), None) => {
// the original field may now be aliased with a name that
matches the
// original qualified name
- let table_ref: TableReference =
field.name().as_str().into();
+ let table_ref =
TableReference::parse_str(field.name().as_str());
Review Comment:
I am not sure if this is correct as it potentially splits on "." -- but I
chose to keep the existing behavior
##########
datafusion/core/tests/sqllogictests/src/insert/mod.rs:
##########
@@ -24,22 +24,21 @@ use datafusion::datasource::MemTable;
use datafusion::prelude::SessionContext;
use datafusion_common::{DFSchema, DataFusionError};
use datafusion_expr::Expr as DFExpr;
-use datafusion_sql::planner::{PlannerContext, SqlToRel};
+use datafusion_sql::planner::{object_name_to_table_reference, PlannerContext,
SqlToRel};
use sqlparser::ast::{Expr, SetExpr, Statement as SQLStatement};
use std::sync::Arc;
-pub async fn insert(ctx: &SessionContext, insert_stmt: &SQLStatement) ->
Result<String> {
+pub async fn insert(ctx: &SessionContext, insert_stmt: SQLStatement) ->
Result<String> {
// First, use sqlparser to get table name and insert values
- let table_name;
+ let table_reference;
Review Comment:
Update the test harness to use the new name handling as well
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1058,7 +1060,7 @@ pub struct CreateCatalogSchema {
#[derive(Clone)]
pub struct DropTable {
/// The table name
- pub name: String,
+ pub name: OwnedTableReference,
Review Comment:
This is the key change -- previously in DataFusion relation (table, view,
etc) were stored as String which was then "parsed" (really split on `.`). This
PR changes the code so the original parsed multi-part TableReference is used
instead
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -915,6 +917,29 @@ message StringifiedPlan {
string plan = 2;
}
+message BareTableReference {
Review Comment:
This is the corresponding serialization support for this feature.
##########
datafusion/sql/src/planner.rs:
##########
@@ -262,7 +264,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
};
Ok(LogicalPlan::CreateMemoryTable(CreateMemoryTable {
- name: name.to_string(),
+ name: object_name_to_table_reference(name)?,
Review Comment:
Rather than encoding the name in a string, it is now encoded as a reference
##########
datafusion/sql/src/utils.rs:
##########
@@ -542,3 +542,11 @@ pub(crate) fn normalize_ident(id: &Ident) -> String {
None => id.value.to_ascii_lowercase(),
}
}
+
+// Normalize an owned identifier to a lowercase string unless the identifier
is quoted.
Review Comment:
I will remove the non-owned variant in a follow on PR -- but I was trying to
minimize the size of this one
##########
datafusion/sql/src/planner.rs:
##########
@@ -1948,37 +1960,41 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})?;
Ok(Expr::ScalarVariable(ty, var_names))
} else {
- match (var_names.pop(), var_names.pop()) {
- (Some(name), Some(relation)) if var_names.is_empty()
=> {
- match schema.field_with_qualified_name(&relation,
&name) {
- Ok(_) => {
- // found an exact match on a qualified
name so this is a table.column identifier
- Ok(Expr::Column(Column {
- relation: Some(relation),
- name,
- }))
- }
- Err(_) => {
- if let Some(field) =
schema.fields().iter().find(|f| f.name().eq(&relation)) {
- // Access to a field of a column which
is a structure, example: SELECT my_struct.key
-
Ok(Expr::GetIndexedField(GetIndexedField::new(
-
Box::new(Expr::Column(field.qualified_column())),
- ScalarValue::Utf8(Some(name)),
- )))
- } else {
- // table.column identifier
- Ok(Expr::Column(Column {
- relation: Some(relation),
- name,
- }))
- }
- }
+ // only support "schema.table" type identifiers here
Review Comment:
this has the same behavior as master but I think the logic is clearer
##########
datafusion/sql/src/planner.rs:
##########
@@ -316,34 +318,40 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
Statement::Drop {
object_type,
if_exists,
- names,
+ mut names,
cascade: _,
restrict: _,
purge: _,
+ } => {
// We don't support cascade and purge for now.
// nor do we support multiple object names
- } => match object_type {
- ObjectType::Table => Ok(LogicalPlan::DropTable(DropTable {
- name: names
- .get(0)
- .ok_or_else(|| ParserError("Missing table
name.".to_string()))?
- .to_string(),
- if_exists,
- schema: DFSchemaRef::new(DFSchema::empty()),
- })),
- ObjectType::View => Ok(LogicalPlan::DropView(DropView {
- name: names
- .get(0)
- .ok_or_else(|| ParserError("Missing table
name.".to_string()))?
- .to_string(),
- if_exists,
- schema: DFSchemaRef::new(DFSchema::empty()),
- })),
- _ => Err(DataFusionError::NotImplemented(
- "Only `DROP TABLE/VIEW ...` statement is supported
currently"
- .to_string(),
- )),
- },
+
+ let name = match names.len() {
+ 0 => Err(ParserError("Missing table
name.".to_string()).into()),
+ 1 => object_name_to_table_reference(names.pop().unwrap()),
+ _ => {
+ Err(ParserError("Multiple objects not
supported".to_string())
Review Comment:
Previously if multiple objects were specified, the second was ignored:
https://github.com/apache/arrow-datafusion/issues/4531
##########
datafusion/sql/src/planner.rs:
##########
@@ -592,6 +602,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let schema = self.build_schema(columns)?;
+ // External tables do not support schemas at the moment, so the name
is just a table name
Review Comment:
the parser returns a String not an ObjectIdentifier, so I kept the behavior
the same
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
