You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2023/02/20 10:03:50 UTC
[GitHub] [arrow-datafusion] Jefffrey opened a new pull request, #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Jefffrey opened a new pull request, #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #4872
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Per original issue, be able to support SQL such as
```sql
select datafusion.public.test.a from test where datafusion.public.test.b = 1
```
This required a larger refactoring specifically around `Column`, to change `relation` from an `Option<String>` to `Option<OwnedTableReference>`, since otherwise can only support a single level of identifier for the relation of a column (unless do some weird stuff like serializing/deserializing into that single String)
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Change `Column` to have `Option<OwnedTableReference>` for `relation` field. Accompanying fixes to rest of codebase for this.
Also fix for the original issue, where in parsing sql compound identifiers can properly identify identifiers which are comprised like `catalog.schema.table.column` which previously wasn't supported.
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Unit test added & fixed, also added test to sqllogictest from the original issue
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
Yes, breaking changes
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1466572908
I plan to merge this PR tomorrow unless anyone else has comments or wants more time to reviwe
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130008949
##########
datafusion/common/src/column.rs:
##########
@@ -53,26 +70,36 @@ impl Column {
/// Deserialize a fully qualified name string into a column
pub fn from_qualified_name(flat_name: impl Into<String>) -> Self {
let flat_name = flat_name.into();
- use sqlparser::tokenizer::Token;
-
- let dialect = sqlparser::dialect::GenericDialect {};
Review Comment:
this is one of the key changes in my mind -- use the full standard identifier normalization rules rather than some custom sqlparser based semantics
##########
datafusion/core/src/execution/context.rs:
##########
@@ -329,19 +329,19 @@ impl SessionContext {
or_replace,
}) => {
let input = Arc::try_unwrap(input).unwrap_or_else(|e| e.as_ref().clone());
- let table = self.table(&name).await;
+ let table = self.table(name.clone()).await;
Review Comment:
I think we can avoid these clones. Here is one way (this is not required as I don't think these codepaths are particularly performance critical)
This compiled locally for me:
```diff
(arrow_dev) alamb@MacBook-Pro-8 arrow-datafusion2 % git diff | cat
git diff | cat
diff --git a/datafusion/common/src/table_reference.rs b/datafusion/common/src/table_reference.rs
index 7fb3dfcc7..a710dfc03 100644
--- a/datafusion/common/src/table_reference.rs
+++ b/datafusion/common/src/table_reference.rs
@@ -189,8 +189,9 @@ impl<'a> TableReference<'a> {
}
}
- /// Converts directly into an [`OwnedTableReference`]
- pub fn to_owned_reference(self) -> OwnedTableReference {
+ /// Converts directly into an [`OwnedTableReference`] without
+ /// copying the underlying Strings
+ pub fn to_owned_reference(&self) -> OwnedTableReference {
match self {
Self::Full {
catalog,
@@ -260,6 +261,12 @@ impl<'a> TableReference<'a> {
}
}
+impl <'a> From<&'a OwnedTableReference> for TableReference<'a> {
+ fn from(value: &'a OwnedTableReference) -> Self {
+ value.to_owned_reference()
+ }
+}
+
/// Parse a `String` into a OwnedTableReference
impl From<String> for OwnedTableReference {
fn from(s: String) -> Self {
diff --git a/datafusion/core/src/execution/context.rs b/datafusion/core/src/execution/context.rs
index f72ea7560..8807721f7 100644
--- a/datafusion/core/src/execution/context.rs
+++ b/datafusion/core/src/execution/context.rs
@@ -329,19 +329,19 @@ impl SessionContext {
or_replace,
}) => {
let input = Arc::try_unwrap(input).unwrap_or_else(|e| e.as_ref().clone());
- let table = self.table(name.clone()).await;
+ let table = self.table(&name).await;
match (if_not_exists, or_replace, table) {
(true, false, Ok(_)) => self.return_empty_dataframe(),
(false, true, Ok(_)) => {
- self.deregister_table(name.clone())?;
+ self.deregister_table(&name)?;
let schema = Arc::new(input.schema().as_ref().into());
let physical = DataFrame::new(self.state(), input);
let batches: Vec<_> = physical.collect_partitioned().await?;
let table = Arc::new(MemTable::try_new(schema, batches)?);
- self.register_table(name.clone(), table)?;
+ self.register_table(&name, table)?;
self.return_empty_dataframe()
}
(true, true, Ok(_)) => Err(DataFusionError::Execution(
@@ -369,15 +369,15 @@ impl SessionContext {
or_replace,
definition,
}) => {
- let view = self.table(name.clone()).await;
+ let view = self.table(&name).await;
match (or_replace, view) {
(true, Ok(_)) => {
- self.deregister_table(name.clone())?;
+ self.deregister_table(&name)?;
let table =
Arc::new(ViewTable::try_new((*input).clone(), definition)?);
- self.register_table(name.clone(), table)?;
+ self.register_table(&name, table)?;
self.return_empty_dataframe()
}
(_, Err(_)) => {
@@ -397,7 +397,7 @@ impl SessionContext {
name, if_exists, ..
}) => {
let result = self
- .find_and_deregister(name.clone(), TableType::Base)
+ .find_and_deregister(&name, TableType::Base)
.await;
match (result, if_exists) {
(Ok(true), _) => self.return_empty_dataframe(),
@@ -412,7 +412,7 @@ impl SessionContext {
name, if_exists, ..
}) => {
let result = self
- .find_and_deregister(name.clone(), TableType::View)
+ .find_and_deregister(&name, TableType::View)
.await;
match (result, if_exists) {
(Ok(true), _) => self.return_empty_dataframe(),
@@ -571,7 +571,7 @@ impl SessionContext {
&self,
cmd: &CreateExternalTable,
) -> Result<DataFrame> {
- let exist = self.table_exist(cmd.name.clone())?;
+ let exist = self.table_exist(cmd.&name)?;
if exist {
match cmd.if_not_exists {
true => return self.return_empty_dataframe(),
@@ -586,7 +586,7 @@ impl SessionContext {
let table_provider: Arc<dyn TableProvider> =
self.create_custom_table(cmd).await?;
- self.register_table(cmd.name.clone(), table_provider)?;
+ self.register_table(cmd.&name, table_provider)?;
self.return_empty_dataframe()
}
(arrow_dev) alamb@MacBook-Pro-8 arrow-datafusion2 %
```
##########
datafusion/common/src/table_reference.rs:
##########
@@ -67,53 +61,53 @@ pub enum TableReference<'a> {
},
}
-/// Represents a path to a table that may require further resolution
-/// that owns the underlying names
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
-pub enum OwnedTableReference {
- /// An unqualified table reference, e.g. "table"
- Bare {
- /// The table name
- table: String,
- },
- /// A partially resolved table reference, e.g. "schema.table"
- Partial {
- /// The schema containing the table
- schema: String,
- /// The table name
- table: String,
- },
- /// A fully resolved table reference, e.g. "catalog.schema.table"
- Full {
- /// The catalog (aka database) containing the table
- catalog: String,
- /// The schema containing the table
- schema: String,
- /// The table name
- table: String,
- },
-}
+pub type OwnedTableReference = TableReference<'static>;
Review Comment:
👍
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -39,6 +39,12 @@ pub fn col(ident: impl Into<Column>) -> Expr {
Expr::Column(ident.into())
}
+/// Create an unqualified column expression from the provided name, without normalizing
+/// the column
Review Comment:
```suggestion
/// the column
///
/// For example `col("A")` refers to a column named 'a' (normalized via SQL rules)
/// but `ident("A")` refers to a column named 'A' (not normalized)
```
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -39,6 +39,12 @@ pub fn col(ident: impl Into<Column>) -> Expr {
Expr::Column(ident.into())
}
+/// Create an unqualified column expression from the provided name, without normalizing
Review Comment:
I think we should also update the docstring of `col` to note that the identifier is normalized according to SQL rules (and in particular, capital letters are reduced to lowercase)
Perhaps some examples would help too:
As in a doc comment showing:
col("A") == col("a")
col(r#""A""#) != col("a")
I can help write this if you would like
##########
datafusion/core/tests/sqllogictests/src/engines/datafusion/insert.rs:
##########
@@ -56,9 +56,9 @@ pub async fn insert(ctx: &SessionContext, insert_stmt: SQLStatement) -> Result<D
}
// Second, get batches in table and destroy the old table
- let mut origin_batches = ctx.table(&table_reference).await?.collect().await?;
- let schema = ctx.table_provider(&table_reference).await?.schema();
- ctx.deregister_table(&table_reference)?;
+ let mut origin_batches = ctx.table(table_reference.clone()).await?.collect().await?;
Review Comment:
```suggestion
let mut origin_batches = ctx.table(&table_reference).await?.collect().await?;
```
If you make the changes to `owned_table_reference` I suggested above
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
If possible, I think errors that are due to user supplied input (like sql queries or inputs to col()) should return something other than `Internal` -- perhaps `DataFusion::Plan`? I think Internal errors are supposed to signal a bug in DataFusion
##########
datafusion/common/src/dfschema.rs:
##########
@@ -677,7 +687,7 @@ mod tests {
// lookup with unqualified name "t1.c0"
let err = schema.index_of_column(&col).err().unwrap();
assert_eq!(
- "Schema error: No field named 't1.c0'. Valid fields are 't1'.'c0', 't1'.'c1'.",
+ r#"Schema error: No field named "t1.c0". Valid fields are "t1"."c0", "t1"."c1"."#,
Review Comment:
I agree we can improve it as a follow on PR
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119896700
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -69,44 +105,100 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})?;
Ok(Expr::ScalarVariable(ty, var_names))
} else {
- // only support "schema.table" type identifiers here
- let (name, relation) = match idents_to_table_reference(
- ids,
- self.options.enable_ident_normalization,
- )? {
- OwnedTableReference::Partial { schema, table } => (table, schema),
- r @ OwnedTableReference::Bare { .. }
- | r @ OwnedTableReference::Full { .. } => {
- return Err(DataFusionError::Plan(format!(
- "Unsupported compound identifier '{r:?}'",
- )));
- }
- };
+ let ids = ids
+ .into_iter()
+ .map(|id| {
+ if self.options.enable_ident_normalization {
+ normalize_ident(id)
+ } else {
+ id.value
+ }
+ })
+ .collect::<Vec<_>>();
- // Try and find the reference in schema
- 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,
- }))
+ // Possibilities we search with, in order from top to bottom for each len:
Review Comment:
Sure thing
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1468709726
Thanks again @Jefffrey -- this is pretty epic. It may be disruptive but I think the new consistency will be worth it
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130598377
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
I made it `Internal` error since `form_identifier(...)` is a private function that is only used by `sql_compound_identifier_to_expr(...)` which already checks that it doesn't pass an empty ident list in (hence calls unwrap on it).
So in normal usage this error can't ever occur. I made it return an error type just in case in future the method is used elsewhere. I'll update the comments a bit to reflect this
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1123708924
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -545,14 +545,22 @@ impl LogicalPlanBuilder {
match (&l.relation, &r.relation) {
(Some(lr), Some(rr)) => {
- let l_is_left =
- self.plan.schema().field_with_qualified_name(lr, &l.name);
- let l_is_right =
- right.schema().field_with_qualified_name(lr, &l.name);
- let r_is_left =
- self.plan.schema().field_with_qualified_name(rr, &r.name);
- let r_is_right =
- right.schema().field_with_qualified_name(rr, &r.name);
+ let l_is_left = self.plan.schema().field_with_qualified_name(
+ &lr.as_table_reference(),
Review Comment:
> Maybe need refactoring around the TableReference structs to try make them more interchangeable, via a common trait possibly
I wonder if we could maybe change TableReference to use `Cow` somehow rather than separate variants 🤔
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130598377
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
I made it `Internal` error since `form_identifier(...)` is a private function that is only used by `sql_compound_identifier_to_expr(...)` and `generate_schema_search_terms(...)` which already check that it doesn't pass an invalid list in (hence calls unwrap on it).
So in normal usage this error can't ever occur. I made it return an error type just in case in future the method is used elsewhere. I'll update the comments a bit to reflect this
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1124358817
##########
datafusion/common/src/dfschema.rs:
##########
@@ -573,21 +580,21 @@ impl ExprSchema for DFSchema {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DFField {
/// Optional qualifier (usually a table or relation name)
- qualifier: Option<String>,
+ qualifier: Option<OwnedTableReference>,
/// Arrow field definition
field: Field,
}
impl DFField {
/// Creates a new `DFField`
- pub fn new(
- qualifier: Option<&str>,
+ pub fn new<R: Into<OwnedTableReference>>(
+ qualifier: Option<R>,
Review Comment:
In the end I opted to leave `new(...)` as is with the `Option<...>` parameter, and introduced a new `new_qualified(...)` to allow specifying without qualifier, without needing to turbofish for a None case
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -69,44 +105,100 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})?;
Ok(Expr::ScalarVariable(ty, var_names))
} else {
- // only support "schema.table" type identifiers here
- let (name, relation) = match idents_to_table_reference(
- ids,
- self.options.enable_ident_normalization,
- )? {
- OwnedTableReference::Partial { schema, table } => (table, schema),
- r @ OwnedTableReference::Bare { .. }
- | r @ OwnedTableReference::Full { .. } => {
- return Err(DataFusionError::Plan(format!(
- "Unsupported compound identifier '{r:?}'",
- )));
- }
- };
+ let ids = ids
+ .into_iter()
+ .map(|id| {
+ if self.options.enable_ident_normalization {
+ normalize_ident(id)
+ } else {
+ id.value
+ }
+ })
+ .collect::<Vec<_>>();
- // Try and find the reference in schema
- 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,
- }))
+ // Possibilities we search with, in order from top to bottom for each len:
Review Comment:
Refactored and added unit tests
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] avantgardnerio commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1453567135
At a high-level, this seems necessary but painful :/
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130665754
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -39,6 +39,12 @@ pub fn col(ident: impl Into<Column>) -> Expr {
Expr::Column(ident.into())
}
+/// Create an unqualified column expression from the provided name, without normalizing
+/// the column
Review Comment:
Updated doc to have example
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119321888
##########
datafusion/common/src/dfschema.rs:
##########
@@ -200,19 +201,19 @@ impl DFSchema {
// field to lookup is qualified.
// current field is qualified and not shared between relations, compare both
// qualifier and name.
- (Some(q), Some(field_q)) => q == field_q && field.name() == name,
+ (Some(q), Some(field_q)) => {
+ q.resolved_eq(&field_q.as_table_reference()) && field.name() == name
+ }
// field to lookup is qualified but current field is unqualified.
(Some(qq), None) => {
// the original field may now be aliased with a name that matches the
// original qualified name
- let table_ref = TableReference::parse_str(field.name().as_str());
- match table_ref {
- TableReference::Partial { schema, table } => {
- schema == qq && table == name
- }
- TableReference::Full { schema, table, .. } => {
- schema == qq && table == name
- }
+ let column = Column::from_qualified_name(field.name());
Review Comment:
this seems like a nicer API for sure
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -545,14 +545,22 @@ impl LogicalPlanBuilder {
match (&l.relation, &r.relation) {
(Some(lr), Some(rr)) => {
- let l_is_left =
- self.plan.schema().field_with_qualified_name(lr, &l.name);
- let l_is_right =
- right.schema().field_with_qualified_name(lr, &l.name);
- let r_is_left =
- self.plan.schema().field_with_qualified_name(rr, &r.name);
- let r_is_right =
- right.schema().field_with_qualified_name(rr, &r.name);
+ let l_is_left = self.plan.schema().field_with_qualified_name(
+ &lr.as_table_reference(),
Review Comment:
I tried playing around with the signature of `field_with_qualified_name` and I couldn't find a way to make `lr` still work. 🤷
##########
datafusion/common/src/table_reference.rs:
##########
@@ -226,57 +258,156 @@ impl<'a> TableReference<'a> {
}
}
-// TODO: remove when can use https://github.com/sqlparser-rs/sqlparser-rs/issues/805
-fn parse_identifiers(s: &str) -> Result<Vec<Ident>> {
- let dialect = GenericDialect;
- let mut parser = Parser::new(&dialect).try_with_sql(s)?;
- let mut idents = vec![];
-
- // expecting at least one word for identifier
- match parser.next_token_no_skip() {
- Some(TokenWithLocation {
- token: Token::Word(w),
- ..
- }) => idents.push(w.to_ident()),
- Some(TokenWithLocation { token, .. }) => {
- return Err(ParserError::ParserError(format!(
- "Unexpected token in identifier: {token}"
- )))?
- }
- None => {
- return Err(ParserError::ParserError(
- "Empty input when parsing identifier".to_string(),
- ))?
+/// Represents a path to a table that may require further resolution
+/// that owns the underlying names
+#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
+pub enum OwnedTableReference {
+ /// An unqualified table reference, e.g. "table"
+ Bare {
+ /// The table name
+ table: String,
+ },
+ /// A partially resolved table reference, e.g. "schema.table"
+ Partial {
+ /// The schema containing the table
+ schema: String,
+ /// The table name
+ table: String,
+ },
+ /// A fully resolved table reference, e.g. "catalog.schema.table"
+ Full {
+ /// The catalog (aka database) containing the table
+ catalog: String,
+ /// The schema containing the table
+ schema: String,
+ /// The table name
+ table: String,
+ },
+}
+
+impl std::fmt::Display for OwnedTableReference {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ OwnedTableReference::Bare { table } => write!(f, "{table}"),
+ OwnedTableReference::Partial { schema, table } => {
+ write!(f, "{schema}.{table}")
+ }
+ OwnedTableReference::Full {
+ catalog,
+ schema,
+ table,
+ } => write!(f, "{catalog}.{schema}.{table}"),
}
- };
-
- while let Some(TokenWithLocation { token, .. }) = parser.next_token_no_skip() {
- match token {
- // ensure that optional period is succeeded by another identifier
- Token::Period => match parser.next_token_no_skip() {
- Some(TokenWithLocation {
- token: Token::Word(w),
- ..
- }) => idents.push(w.to_ident()),
- Some(TokenWithLocation { token, .. }) => {
- return Err(ParserError::ParserError(format!(
- "Unexpected token following period in identifier: {token}"
- )))?
- }
- None => {
- return Err(ParserError::ParserError(
- "Trailing period in identifier".to_string(),
- ))?
- }
+ }
+}
+
+impl OwnedTableReference {
+ /// Return a `TableReference` view of this `OwnedTableReference`
+ pub fn as_table_reference(&self) -> TableReference<'_> {
+ match self {
+ Self::Bare { table } => TableReference::Bare {
+ table: table.into(),
+ },
+ Self::Partial { schema, table } => TableReference::Partial {
+ schema: schema.into(),
+ table: table.into(),
+ },
+ Self::Full {
+ catalog,
+ schema,
+ table,
+ } => TableReference::Full {
+ catalog: catalog.into(),
+ schema: schema.into(),
+ table: table.into(),
},
- _ => {
- return Err(ParserError::ParserError(format!(
- "Unexpected token in identifier: {token}"
- )))?
+ }
+ }
+
+ /// Retrieve the actual table name, regardless of qualification
+ pub fn table(&self) -> &str {
+ match self {
+ Self::Full { table, .. }
+ | Self::Partial { table, .. }
+ | Self::Bare { table } => table,
+ }
+ }
+
+ /// Forms a string where the identifiers are quoted
+ pub fn to_quoted_string(&self) -> String {
+ match self {
+ OwnedTableReference::Bare { table } => quote_identifier(table),
+ OwnedTableReference::Partial { schema, table } => {
+ format!("{}.{}", quote_identifier(schema), quote_identifier(table))
}
+ OwnedTableReference::Full {
+ catalog,
+ schema,
+ table,
+ } => format!(
+ "{}.{}.{}",
+ quote_identifier(catalog),
+ quote_identifier(schema),
+ quote_identifier(table)
+ ),
}
}
- Ok(idents)
+}
+
+impl PartialEq<TableReference<'_>> for OwnedTableReference {
+ fn eq(&self, other: &TableReference<'_>) -> bool {
+ self.as_table_reference().eq(other)
+ }
+}
+
+impl PartialEq<OwnedTableReference> for TableReference<'_> {
+ fn eq(&self, other: &OwnedTableReference) -> bool {
+ self.eq(&other.as_table_reference())
+ }
+}
+
+/// Parse a `&str` into a OwnedTableReference
+impl From<&str> for OwnedTableReference {
+ fn from(s: &str) -> Self {
+ let table_reference: TableReference = s.into();
+ table_reference.to_owned_reference()
+ }
+}
+
+/// Parse a `String` into a OwnedTableReference
+impl From<String> for OwnedTableReference {
+ fn from(s: String) -> Self {
+ Self::from(s.as_str())
+ }
+}
+
+/// Parse a `&String` into a OwnedTableReference
+impl From<&String> for OwnedTableReference {
+ fn from(s: &String) -> Self {
+ Self::from(s.as_str())
+ }
+}
+
+/// Parse a `&String` into a OwnedTableReference
+impl From<&OwnedTableReference> for OwnedTableReference {
+ fn from(s: &OwnedTableReference) -> Self {
+ s.clone()
+ }
+}
+
+/// Parse a `TableReference` into a OwnedTableReference
+impl From<&'_ TableReference<'_>> for OwnedTableReference {
Review Comment:
👍
##########
datafusion/optimizer/src/push_down_projection.rs:
##########
@@ -497,8 +494,10 @@ fn push_down_scan(
let schema = scan.source.schema();
let mut projection: BTreeSet<usize> = required_columns
.iter()
+ // TODO: change scan.table_name from String?
Review Comment:
maybe worth a follow on ticket?
##########
docs/source/user-guide/example-usage.md:
##########
@@ -118,8 +118,8 @@ async fn main() -> datafusion::error::Result<()> {
let ctx = SessionContext::new();
let df = ctx.read_csv("tests/data/capitalized_example.csv", CsvReadOptions::new()).await?;
- let df = df.filter(col("A").lt_eq(col("c")))?
- .aggregate(vec![col("A")], vec![min(col("b"))])?
+ let df = df.filter(col("\"A\"").lt_eq(col("c")))?
+ .aggregate(vec![col("\"A\"")], vec![min(col("b"))])?
.limit(0, Some(100))?;
Review Comment:
This is probably a pretty major change for people (that `col()` will now parse / normalize the column names according to SQL semantics).
However, it does have the nice side benefit that the handling of `.` is no longer treated differently in `col()`
What do you think about possibly
1. changing `col` so that it forms a unqualified reference always (aka does not respect `.`)
2. adding some new function like `ident()` or `parse_col()` that treats column like a sql identifier and correctly splits based on `.` etc?
##########
datafusion/common/src/dfschema.rs:
##########
@@ -677,7 +687,7 @@ mod tests {
// lookup with unqualified name "t1.c0"
let err = schema.index_of_column(&col).err().unwrap();
assert_eq!(
- "Schema error: No field named 't1.c0'. Valid fields are 't1'.'c0', 't1'.'c1'.",
+ r#"Schema error: No field named "t1.c0". Valid fields are "t1"."c0", "t1"."c1"."#,
Review Comment:
I agree the new change is better.
We can maybe go a step farther and only put `"` if the identifier would be normalized (e.g. it has something other than lowercase ascii letters) -- as a potential follow on PR
##########
datafusion/common/src/dfschema.rs:
##########
@@ -573,21 +580,21 @@ impl ExprSchema for DFSchema {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DFField {
/// Optional qualifier (usually a table or relation name)
- qualifier: Option<String>,
+ qualifier: Option<OwnedTableReference>,
/// Arrow field definition
field: Field,
}
impl DFField {
/// Creates a new `DFField`
- pub fn new(
- qualifier: Option<&str>,
+ pub fn new<R: Into<OwnedTableReference>>(
+ qualifier: Option<R>,
Review Comment:
> or maintain old method and create a new one?)
Yeah, I agree it is strange to see `field_not_found::<&str>` and `DFField::new::<&str>` when there is no qualifier
Since all sites with `None` as the qualifier are going to need to be changed anyways, what about making different functions for creating qualified and unqualified fields? Something like this:
```rust
impl DFField {
fn new<R: Into<OwnedTableReference>>(
qualifier:R ,
name: &str,
data_type: DataType,
nullable: bool,
) -> Self {..}
fn new_unqualified(
name: &str,
data_type: DataType,
nullable: bool,
) -> Self {..}
}
```
?
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -69,44 +105,100 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})?;
Ok(Expr::ScalarVariable(ty, var_names))
} else {
- // only support "schema.table" type identifiers here
- let (name, relation) = match idents_to_table_reference(
- ids,
- self.options.enable_ident_normalization,
- )? {
- OwnedTableReference::Partial { schema, table } => (table, schema),
- r @ OwnedTableReference::Bare { .. }
- | r @ OwnedTableReference::Full { .. } => {
- return Err(DataFusionError::Plan(format!(
- "Unsupported compound identifier '{r:?}'",
- )));
- }
- };
+ let ids = ids
+ .into_iter()
+ .map(|id| {
+ if self.options.enable_ident_normalization {
+ normalize_ident(id)
+ } else {
+ id.value
+ }
+ })
+ .collect::<Vec<_>>();
- // Try and find the reference in schema
- 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,
- }))
+ // Possibilities we search with, in order from top to bottom for each len:
Review Comment:
Could you write some more unit tests for these cases (the comments are great, but it would be better if they were tests so they can't get out of sync as easily)
##########
datafusion/common/src/column.rs:
##########
@@ -27,15 +28,18 @@ use std::sync::Arc;
/// A named reference to a qualified field in a schema.
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
- /// relation/table name.
- pub relation: Option<String>,
+ /// relation/table reference.
+ pub relation: Option<OwnedTableReference>,
/// field/column name.
pub name: String,
}
impl Column {
/// Create Column from optional qualifier and name
- pub fn new(relation: Option<impl Into<String>>, name: impl Into<String>) -> Self {
+ pub fn new(
+ relation: Option<impl Into<OwnedTableReference>>,
Review Comment:
maybe we can update the docstring of this function to explain the new behavior (aka that relations will be parsed and normalized
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130636567
##########
datafusion/optimizer/src/push_down_projection.rs:
##########
@@ -497,8 +494,10 @@ fn push_down_scan(
let schema = scan.source.schema();
let mut projection: BTreeSet<usize> = required_columns
.iter()
+ // TODO: change scan.table_name from String?
Review Comment:
ticket raised: https://github.com/apache/arrow-datafusion/issues/5522
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130598377
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
I made it `Internal` error since `form_identifier(...)` is a private function that is only used by `sql_compound_identifier_to_expr(...)` and `generate_schema_search_terms(...)` which already check that it doesn't pass an invalid list in (hence calls unwrap on it).
So in normal usage this error can't ever occur. I made it return an error type just in case in future the method is used elsewhere. I'll update the comments a bit to reflect this
Edit: comments updated
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119892376
##########
datafusion/common/src/dfschema.rs:
##########
@@ -573,21 +580,21 @@ impl ExprSchema for DFSchema {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DFField {
/// Optional qualifier (usually a table or relation name)
- qualifier: Option<String>,
+ qualifier: Option<OwnedTableReference>,
/// Arrow field definition
field: Field,
}
impl DFField {
/// Creates a new `DFField`
- pub fn new(
- qualifier: Option<&str>,
+ pub fn new<R: Into<OwnedTableReference>>(
+ qualifier: Option<R>,
Review Comment:
An issue with this approach is that there are still use cases for an Option<...> parameter, such as when the caller already possesses an `Option<OwnedTableReference>` (such as the `relation` field of `Column`)
This would force them to destructure that Option manually to call the correct version, `new(...)` or `new_unqualified(...)`, which makes it more verbose for the caller
I suppose could add yet another version of new... I'm not sure I like that approach either though
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1111730595
##########
datafusion/common/src/dfschema.rs:
##########
@@ -573,21 +580,21 @@ impl ExprSchema for DFSchema {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DFField {
/// Optional qualifier (usually a table or relation name)
- qualifier: Option<String>,
+ qualifier: Option<OwnedTableReference>,
/// Arrow field definition
field: Field,
}
impl DFField {
/// Creates a new `DFField`
- pub fn new(
- qualifier: Option<&str>,
+ pub fn new<R: Into<OwnedTableReference>>(
+ qualifier: Option<R>,
Review Comment:
originally had this way to try ease the breaking change, since would still support Option<&str>, but can see will cause pain if trying to pass in just None itself. maybe its better to just have this as Option<OwnedTableReference> and have the callers call into() (or maintain old method and create a new one?)
##########
datafusion/common/src/error.rs:
##########
@@ -120,29 +121,29 @@ macro_rules! plan_err {
#[derive(Debug)]
pub enum SchemaError {
/// Schema contains a (possibly) qualified and unqualified field with same unqualified name
- AmbiguousReference {
- qualifier: Option<String>,
+ AmbiguousReference { field: Column },
+ /// Schema contains duplicate qualified field name
+ DuplicateQualifiedField {
+ qualifier: OwnedTableReference,
name: String,
},
- /// Schema contains duplicate qualified field name
- DuplicateQualifiedField { qualifier: String, name: String },
/// Schema contains duplicate unqualified field name
DuplicateUnqualifiedField { name: String },
/// No field with this name
FieldNotFound {
- field: Column,
+ field: Box<Column>,
Review Comment:
otherwise clippy lint about large Err variant appeared
##########
datafusion/common/src/dfschema.rs:
##########
@@ -677,7 +687,7 @@ mod tests {
// lookup with unqualified name "t1.c0"
let err = schema.index_of_column(&col).err().unwrap();
assert_eq!(
- "Schema error: No field named 't1.c0'. Valid fields are 't1'.'c0', 't1'.'c1'.",
+ r#"Schema error: No field named "t1.c0". Valid fields are "t1"."c0", "t1"."c1"."#,
Review Comment:
this was a separate change i decided to add, mainly since i believe having quoted identifiers allows for more robust messages as itll properly account for any special characters within and allows users to clearly delineate identifiers in that case
e.g. in old way a user could have an ident like `t1'.'t2` which would show in error as `'t1'.'t2'` even though it is a single ident, it looks like two
##########
datafusion/common/src/column.rs:
##########
@@ -27,15 +28,18 @@ use std::sync::Arc;
/// A named reference to a qualified field in a schema.
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
- /// relation/table name.
- pub relation: Option<String>,
+ /// relation/table reference.
+ pub relation: Option<OwnedTableReference>,
/// field/column name.
pub name: String,
}
impl Column {
/// Create Column from optional qualifier and name
- pub fn new(relation: Option<impl Into<String>>, name: impl Into<String>) -> Self {
+ pub fn new(
+ relation: Option<impl Into<OwnedTableReference>>,
Review Comment:
by having this as Into<OwnedTableReference>, means if passing in a string it will be parsed. this has the side-effect of if passing in just a string like `C1` for the column name, itll be normalized to `c1` (wheres previously it wasn't, since was passed straight through)
if this breaking behaviour is too large, can keep the old method (and just create an `OwnedTableReference::Bare` from it) and have a new method which accepts `Option<OwnedTableReference>` for the relation
see documentation change that was needed due to `col(...)` behaviour changing for reference
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -69,44 +105,100 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
})?;
Ok(Expr::ScalarVariable(ty, var_names))
} else {
- // only support "schema.table" type identifiers here
- let (name, relation) = match idents_to_table_reference(
- ids,
- self.options.enable_ident_normalization,
- )? {
- OwnedTableReference::Partial { schema, table } => (table, schema),
- r @ OwnedTableReference::Bare { .. }
- | r @ OwnedTableReference::Full { .. } => {
- return Err(DataFusionError::Plan(format!(
- "Unsupported compound identifier '{r:?}'",
- )));
- }
- };
+ let ids = ids
+ .into_iter()
+ .map(|id| {
+ if self.options.enable_ident_normalization {
+ normalize_ident(id)
+ } else {
+ id.value
+ }
+ })
+ .collect::<Vec<_>>();
- // Try and find the reference in schema
- 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,
- }))
+ // Possibilities we search with, in order from top to bottom for each len:
Review Comment:
main fix for original issue is in here
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1452567170
So in my opinion this PR, while a non trivial breaking change, results in a better outcome -- however, before we merge it let's solict additional community feedback.
I posted a note in the slack channel and the dev list https://lists.apache.org/thread/m42rk0sbh88bnbgnx83wshhm97wrxjf7
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1445353798
I plan to review this PR carefully tomorrow. I am sorry for the delay
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130673179
##########
datafusion/core/src/execution/context.rs:
##########
@@ -329,19 +329,19 @@ impl SessionContext {
or_replace,
}) => {
let input = Arc::try_unwrap(input).unwrap_or_else(|e| e.as_ref().clone());
- let table = self.table(&name).await;
+ let table = self.table(name.clone()).await;
Review Comment:
Yeah this looks good, as it looks better without the clone, however I think `to_owned_reference(...)` will clone the underlying strings anyway:
https://github.com/Jefffrey/arrow-datafusion/blob/57ce66c24858cedac4f2c56194ea72f84d7be2f5/datafusion/common/src/table_reference.rs#L192-L212
Edit: implemented something similar
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119892376
##########
datafusion/common/src/dfschema.rs:
##########
@@ -573,21 +580,21 @@ impl ExprSchema for DFSchema {
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DFField {
/// Optional qualifier (usually a table or relation name)
- qualifier: Option<String>,
+ qualifier: Option<OwnedTableReference>,
/// Arrow field definition
field: Field,
}
impl DFField {
/// Creates a new `DFField`
- pub fn new(
- qualifier: Option<&str>,
+ pub fn new<R: Into<OwnedTableReference>>(
+ qualifier: Option<R>,
Review Comment:
An issue with this approach is that there are still use cases for an Option<...> parameter, such as when the caller already possesses an `Option<OwnedTableReference>` (such as the `relation` field of `Column`)
This would force them to destructure that Option manually to call the correct version, `new(...)` or `new_unqualified(...)`, which makes it more verbose for the caller
I suppose could add yet another version of new to accept an Option... I'm not sure I like that approach either though
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119909964
##########
docs/source/user-guide/example-usage.md:
##########
@@ -118,8 +118,8 @@ async fn main() -> datafusion::error::Result<()> {
let ctx = SessionContext::new();
let df = ctx.read_csv("tests/data/capitalized_example.csv", CsvReadOptions::new()).await?;
- let df = df.filter(col("A").lt_eq(col("c")))?
- .aggregate(vec![col("A")], vec![min(col("b"))])?
+ let df = df.filter(col("\"A\"").lt_eq(col("c")))?
+ .aggregate(vec![col("\"A\"")], vec![min(col("b"))])?
.limit(0, Some(100))?;
Review Comment:
Yeah that would have the benefit of being less breaking and being a bit more explicit in what is happening. `parse_col` would make it more explicit though I'm not really a fan of that name, feels a bit clunky.
For what its worth Spark does seem to be able to parse the input to their `col` function, instead of taking it wholly unqualified I believe
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119903703
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -545,14 +545,22 @@ impl LogicalPlanBuilder {
match (&l.relation, &r.relation) {
(Some(lr), Some(rr)) => {
- let l_is_left =
- self.plan.schema().field_with_qualified_name(lr, &l.name);
- let l_is_right =
- right.schema().field_with_qualified_name(lr, &l.name);
- let r_is_left =
- self.plan.schema().field_with_qualified_name(rr, &r.name);
- let r_is_right =
- right.schema().field_with_qualified_name(rr, &r.name);
+ let l_is_left = self.plan.schema().field_with_qualified_name(
+ &lr.as_table_reference(),
Review Comment:
Yeah was thinking maybe having something like `Into<TableReference>` or the like.
Related, but I feel TableReference and its related structs OwnedTableReference and ResolvedTableReference got a bit out of hand, since they seem all very similar and have conversions between them, but still are quite separate. I think I initially had `field_with_qualified_name` accept a reference to an OwnedTableReference but changed it to TableReference as that wouldn't require owning a bunch of strings if you just wanted to pass in a reference, but can see it doesn't really result in a nice API imo
Maybe need refactoring around the TableReference structs to try make them more interchangeable, via a common trait possibly
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1452576662
I agree that this is a good change and the API change is OK with us. However, honestly I can not confidently tell that it won't mess with others' usage patterns. I think waiting for some more community feedback is a good idea.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1129296956
##########
docs/source/user-guide/example-usage.md:
##########
@@ -118,8 +118,8 @@ async fn main() -> datafusion::error::Result<()> {
let ctx = SessionContext::new();
let df = ctx.read_csv("tests/data/capitalized_example.csv", CsvReadOptions::new()).await?;
- let df = df.filter(col("A").lt_eq(col("c")))?
- .aggregate(vec![col("A")], vec![min(col("b"))])?
+ let df = df.filter(col("\"A\"").lt_eq(col("c")))?
+ .aggregate(vec![col("\"A\"")], vec![min(col("b"))])?
.limit(0, Some(100))?;
Review Comment:
I've added the `ident()` expr function and updated the `user-guide/example-usage.md` doc
Let me know if there are other places I need to change/add docs for
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1124359082
##########
datafusion/common/src/column.rs:
##########
@@ -27,15 +28,18 @@ use std::sync::Arc;
/// A named reference to a qualified field in a schema.
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
- /// relation/table name.
- pub relation: Option<String>,
+ /// relation/table reference.
+ pub relation: Option<OwnedTableReference>,
/// field/column name.
pub name: String,
}
impl Column {
/// Create Column from optional qualifier and name
- pub fn new(relation: Option<impl Into<String>>, name: impl Into<String>) -> Self {
+ pub fn new(
+ relation: Option<impl Into<OwnedTableReference>>,
Review Comment:
Doc updated
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119897074
##########
datafusion/common/src/column.rs:
##########
@@ -27,15 +28,18 @@ use std::sync::Arc;
/// A named reference to a qualified field in a schema.
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
- /// relation/table name.
- pub relation: Option<String>,
+ /// relation/table reference.
+ pub relation: Option<OwnedTableReference>,
/// field/column name.
pub name: String,
}
impl Column {
/// Create Column from optional qualifier and name
- pub fn new(relation: Option<impl Into<String>>, name: impl Into<String>) -> Self {
+ pub fn new(
+ relation: Option<impl Into<OwnedTableReference>>,
Review Comment:
Sure thing
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130665330
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -39,6 +39,12 @@ pub fn col(ident: impl Into<Column>) -> Expr {
Expr::Column(ident.into())
}
+/// Create an unqualified column expression from the provided name, without normalizing
Review Comment:
Doc updated
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130642881
##########
datafusion/common/src/dfschema.rs:
##########
@@ -677,7 +687,7 @@ mod tests {
// lookup with unqualified name "t1.c0"
let err = schema.index_of_column(&col).err().unwrap();
assert_eq!(
- "Schema error: No field named 't1.c0'. Valid fields are 't1'.'c0', 't1'.'c1'.",
+ r#"Schema error: No field named "t1.c0". Valid fields are "t1"."c0", "t1"."c1"."#,
Review Comment:
raised ticket: https://github.com/apache/arrow-datafusion/issues/5523
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1131178045
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
thank you 👍 makes sense
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1130673179
##########
datafusion/core/src/execution/context.rs:
##########
@@ -329,19 +329,19 @@ impl SessionContext {
or_replace,
}) => {
let input = Arc::try_unwrap(input).unwrap_or_else(|e| e.as_ref().clone());
- let table = self.table(&name).await;
+ let table = self.table(name.clone()).await;
Review Comment:
Yeah this looks good, as it looks better without the clone, however I think `to_owned_reference(...)` will clone the underlying strings anyway:
https://github.com/Jefffrey/arrow-datafusion/blob/57ce66c24858cedac4f2c56194ea72f84d7be2f5/datafusion/common/src/table_reference.rs#L192-L212
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1119896440
##########
datafusion/common/src/dfschema.rs:
##########
@@ -677,7 +687,7 @@ mod tests {
// lookup with unqualified name "t1.c0"
let err = schema.index_of_column(&col).err().unwrap();
assert_eq!(
- "Schema error: No field named 't1.c0'. Valid fields are 't1'.'c0', 't1'.'c1'.",
+ r#"Schema error: No field named "t1.c0". Valid fields are "t1"."c0", "t1"."c1"."#,
Review Comment:
Yeah this did come to mind, but:
- Either have to store alongside the name that it was normalized
- Or each time quoting string have to do a pass over it to check for special characters/uppercase characters
1st one I don't really like as have to add more to TableReference (and related structs)
2nd one would be simpler to implement, maybe I'm overthinking the impact of the overhead 😅
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1124338686
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -545,14 +545,22 @@ impl LogicalPlanBuilder {
match (&l.relation, &r.relation) {
(Some(lr), Some(rr)) => {
- let l_is_left =
- self.plan.schema().field_with_qualified_name(lr, &l.name);
- let l_is_right =
- right.schema().field_with_qualified_name(lr, &l.name);
- let r_is_left =
- self.plan.schema().field_with_qualified_name(rr, &r.name);
- let r_is_right =
- right.schema().field_with_qualified_name(rr, &r.name);
+ let l_is_left = self.plan.schema().field_with_qualified_name(
+ &lr.as_table_reference(),
Review Comment:
How do you mean?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1128452662
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -545,14 +545,22 @@ impl LogicalPlanBuilder {
match (&l.relation, &r.relation) {
(Some(lr), Some(rr)) => {
- let l_is_left =
- self.plan.schema().field_with_qualified_name(lr, &l.name);
- let l_is_right =
- right.schema().field_with_qualified_name(lr, &l.name);
- let r_is_left =
- self.plan.schema().field_with_qualified_name(rr, &r.name);
- let r_is_right =
- right.schema().field_with_qualified_name(rr, &r.name);
+ let l_is_left = self.plan.schema().field_with_qualified_name(
+ &lr.as_table_reference(),
Review Comment:
I was thinking about removing OwnedTableReference -- I started to sketch out what this might look like in https://github.com/apache/arrow-datafusion/pull/5508
Working with the structures then I think becomes less cumbersome
##########
docs/source/user-guide/example-usage.md:
##########
@@ -118,8 +118,8 @@ async fn main() -> datafusion::error::Result<()> {
let ctx = SessionContext::new();
let df = ctx.read_csv("tests/data/capitalized_example.csv", CsvReadOptions::new()).await?;
- let df = df.filter(col("A").lt_eq(col("c")))?
- .aggregate(vec![col("A")], vec![min(col("b"))])?
+ let df = df.filter(col("\"A\"").lt_eq(col("c")))?
+ .aggregate(vec![col("\"A\"")], vec![min(col("b"))])?
.limit(0, Some(100))?;
Review Comment:
How about we simply add an `ident()` function that doesn't parse at all? I can help do that
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#discussion_r1131178045
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -160,3 +180,243 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
)))
}
}
+
+// (relation, column name)
+fn form_identifier(idents: &[String]) -> Result<(Option<TableReference>, &String)> {
+ match idents.len() {
+ 1 => Ok((None, &idents[0])),
+ 2 => Ok((
+ Some(TableReference::Bare {
+ table: (&idents[0]).into(),
+ }),
+ &idents[1],
+ )),
+ 3 => Ok((
+ Some(TableReference::Partial {
+ schema: (&idents[0]).into(),
+ table: (&idents[1]).into(),
+ }),
+ &idents[2],
+ )),
+ 4 => Ok((
+ Some(TableReference::Full {
+ catalog: (&idents[0]).into(),
+ schema: (&idents[1]).into(),
+ table: (&idents[2]).into(),
+ }),
+ &idents[3],
+ )),
+ _ => Err(DataFusionError::Internal(format!(
+ "Incorrect number of identifiers: {}",
+ idents.len()
+ ))),
+ }
+}
+
+fn search_dfschema<'ids, 'schema>(
+ ids: &'ids [String],
+ schema: &'schema DFSchema,
+) -> Option<(&'schema DFField, &'ids [String])> {
+ generate_schema_search_terms(ids).find_map(|(qualifier, column, nested_names)| {
+ let field = schema.field_with_name(qualifier.as_ref(), column).ok();
+ field.map(|f| (f, nested_names))
+ })
+}
+
+// Possibilities we search with, in order from top to bottom for each len:
+//
+// len = 2:
+// 1. (table.column)
+// 2. (column).nested
+//
+// len = 3:
+// 1. (schema.table.column)
+// 2. (table.column).nested
+// 3. (column).nested1.nested2
+//
+// len = 4:
+// 1. (catalog.schema.table.column)
+// 2. (schema.table.column).nested1
+// 3. (table.column).nested1.nested2
+// 4. (column).nested1.nested2.nested3
+//
+// len = 5:
+// 1. (catalog.schema.table.column).nested
+// 2. (schema.table.column).nested1.nested2
+// 3. (table.column).nested1.nested2.nested3
+// 4. (column).nested1.nested2.nested3.nested4
+//
+// len > 5:
+// 1. (catalog.schema.table.column).nested[.nestedN]+
+// 2. (schema.table.column).nested1.nested2[.nestedN]+
+// 3. (table.column).nested1.nested2.nested3[.nestedN]+
+// 4. (column).nested1.nested2.nested3.nested4[.nestedN]+
+fn generate_schema_search_terms(
+ ids: &[String],
+) -> impl Iterator<Item = (Option<TableReference>, &String, &[String])> {
+ // take at most 4 identifiers to form a Column to search with
+ // - 1 for the column name
+ // - 0 to 3 for the TableReference
+ let bound = ids.len().min(4);
+ // search terms from most specific to least specific
+ (0..bound).rev().map(|i| {
+ let nested_names_index = i + 1;
+ let qualifier_and_column = &ids[0..nested_names_index];
+ let (relation, column_name) = form_identifier(qualifier_and_column).unwrap();
+ (relation, column_name, &ids[nested_names_index..])
+ })
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ // testing according to documentation of generate_schema_search_terms function
+ // where ensure generated search terms are in correct order with correct values
+ fn test_generate_schema_search_terms() -> Result<()> {
+ type ExpectedItem = (
+ Option<TableReference<'static>>,
+ &'static str,
+ &'static [&'static str],
+ );
+ fn assert_vec_eq(
+ expected: Vec<ExpectedItem>,
+ actual: Vec<(Option<TableReference>, &String, &[String])>,
+ ) {
+ for (expected, actual) in expected.into_iter().zip(actual) {
+ assert_eq!(expected.0, actual.0, "qualifier");
+ assert_eq!(expected.1, actual.1, "column name");
+ assert_eq!(expected.2, actual.2, "nested names");
+ }
+ }
+
+ let actual = generate_schema_search_terms(&[]).collect::<Vec<_>>();
+ assert!(actual.is_empty());
+
+ let ids = vec!["a".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![(None, "a", &[])];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::bare("a")), "b", &[]),
+ (None, "a", &["b"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec!["a".to_string(), "b".to_string(), "c".to_string()];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::partial("a", "b")), "c", &[]),
+ (Some(TableReference::bare("a")), "b", &["c"]),
+ (None, "a", &["b", "c"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &[]),
+ (Some(TableReference::partial("a", "b")), "c", &["d"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d"]),
+ (None, "a", &["b", "c", "d"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e"]),
+ (Some(TableReference::partial("a", "b")), "c", &["d", "e"]),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e"]),
+ (None, "a", &["b", "c", "d", "e"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ let ids = vec![
+ "a".to_string(),
+ "b".to_string(),
+ "c".to_string(),
+ "d".to_string(),
+ "e".to_string(),
+ "f".to_string(),
+ ];
+ let actual = generate_schema_search_terms(&ids).collect::<Vec<_>>();
+ let expected: Vec<ExpectedItem> = vec![
+ (Some(TableReference::full("a", "b", "c")), "d", &["e", "f"]),
+ (
+ Some(TableReference::partial("a", "b")),
+ "c",
+ &["d", "e", "f"],
+ ),
+ (Some(TableReference::bare("a")), "b", &["c", "d", "e", "f"]),
+ (None, "a", &["b", "c", "d", "e", "f"]),
+ ];
+ assert_vec_eq(expected, actual);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_form_identifier() -> Result<()> {
+ let err = form_identifier(&[]).expect_err("empty identifiers didn't fail");
+ let expected = "Internal error: Incorrect number of identifiers: 0. \
Review Comment:
thank you 👍 makessense
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1468246913
Benchmark runs are scheduled for baseline = 612eb1d0ce338af7980fa906df8796eb47c4be44 and contender = 4d07360e6f67012571237ab1a773ac1fec7122d9. 4d07360e6f67012571237ab1a773ac1fec7122d9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/99e3d44022df48f1bde4e59be96f79e5...ec167f114734445b9a6e7001ad2d1e10/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0d64c4eabb9e4c7a9fb9cbbdb1fa7f8d...9301b6a174fa4c7faba25db08d986800/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/efd62816e03d4f509fcfae4c8863d66d...5fa577ba7ba145c7846c902aaad16740/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/aef04ea47bb244ffb4df4242828ff339...38447e8810d1439490500e9b61ba841d/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jefffrey commented on pull request #5343: Support catalog.schema.table.column in SQL SELECT and WHERE
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5343:
URL: https://github.com/apache/arrow-datafusion/pull/5343#issuecomment-1460029211
I've done the changes for `OwnedTableReference` per your suggestion @alamb
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org