From dfeab5ed754ae52fa435c8db7b0c77c01d5e9f08 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Sat, 15 Apr 2023 12:08:49 -0500 Subject: [PATCH 1/4] Use OwnedTableReference for subquery aliases --- datafusion/expr/src/logical_plan/builder.rs | 4 ++-- datafusion/expr/src/logical_plan/plan.rs | 16 ++++++++-------- .../optimizer/src/analyzer/inline_table_scan.rs | 14 ++++---------- datafusion/optimizer/src/decorrelate_where_in.rs | 2 +- .../optimizer/src/scalar_subquery_to_join.rs | 2 +- 5 files changed, 16 insertions(+), 22 deletions(-) diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index d16b7cd8c5e9..ec2a27542c56 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -381,7 +381,7 @@ impl LogicalPlanBuilder { } /// Apply an alias - pub fn alias(self, alias: impl Into) -> Result { + pub fn alias(self, alias: impl Into) -> Result { Ok(Self::from(subquery_alias(self.plan, alias)?)) } @@ -1236,7 +1236,7 @@ pub fn project( /// Create a SubqueryAlias to wrap a LogicalPlan. pub fn subquery_alias( plan: LogicalPlan, - alias: impl Into, + alias: impl Into, ) -> Result { Ok(LogicalPlan::SubqueryAlias(SubqueryAlias::try_new( plan, alias, diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 1e85e1368434..662c403f024a 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -36,7 +36,7 @@ use datafusion_common::tree_node::{ }; use datafusion_common::{ plan_err, Column, DFSchema, DFSchemaRef, DataFusionError, OwnedTableReference, - Result, ScalarValue, TableReference, + Result, ScalarValue, }; use std::collections::{HashMap, HashSet}; use std::fmt::{self, Debug, Display, Formatter}; @@ -1304,20 +1304,20 @@ pub struct SubqueryAlias { /// The incoming logical plan pub input: Arc, /// The alias for the input relation - pub alias: String, + pub alias: OwnedTableReference, /// The schema with qualified field names pub schema: DFSchemaRef, } impl SubqueryAlias { - pub fn try_new(plan: LogicalPlan, alias: impl Into) -> Result { + pub fn try_new( + plan: LogicalPlan, + alias: impl Into, + ) -> Result { let alias = alias.into(); - let table_ref = TableReference::bare(&alias); let schema: Schema = plan.schema().as_ref().clone().into(); - let schema = DFSchemaRef::new(DFSchema::try_from_qualified_schema( - table_ref.to_owned_reference(), - &schema, - )?); + let schema = + DFSchemaRef::new(DFSchema::try_from_qualified_schema(&alias, &schema)?); Ok(SubqueryAlias { input: Arc::new(plan), alias, diff --git a/datafusion/optimizer/src/analyzer/inline_table_scan.rs b/datafusion/optimizer/src/analyzer/inline_table_scan.rs index 550d095f3e4b..473fe33deccf 100644 --- a/datafusion/optimizer/src/analyzer/inline_table_scan.rs +++ b/datafusion/optimizer/src/analyzer/inline_table_scan.rs @@ -64,16 +64,10 @@ fn analyze_internal(plan: LogicalPlan) -> Result> { let projection_exprs = generate_projection_expr(&projection, sub_plan)?; let plan = LogicalPlanBuilder::from(sub_plan.clone()) .project(projection_exprs)? - // Since this This is creating a subquery like: - //```sql - // ... - // FROM as "table_name" - // ``` - // - // it doesn't make sense to have a qualified - // reference (e.g. "foo"."bar") -- this convert to - // string - .alias(table_name.to_string())? + // This will ensure that the reference to the inlined table + // remains the same ensuring we don't have to change any of the + // parent nodes that reference this table. + .alias(table_name)? .build()?; Transformed::Yes(plan) } diff --git a/datafusion/optimizer/src/decorrelate_where_in.rs b/datafusion/optimizer/src/decorrelate_where_in.rs index d89fd935be60..c4d46d93c007 100644 --- a/datafusion/optimizer/src/decorrelate_where_in.rs +++ b/datafusion/optimizer/src/decorrelate_where_in.rs @@ -183,7 +183,7 @@ fn optimize_where_in( let right = LogicalPlanBuilder::from(subquery_input) .project(projection_exprs)? - .alias(&subquery_alias)? + .alias(subquery_alias.clone())? .build()?; // join our sub query into the main plan diff --git a/datafusion/optimizer/src/scalar_subquery_to_join.rs b/datafusion/optimizer/src/scalar_subquery_to_join.rs index df0b9245faec..d88aca32cb9e 100644 --- a/datafusion/optimizer/src/scalar_subquery_to_join.rs +++ b/datafusion/optimizer/src/scalar_subquery_to_join.rs @@ -266,7 +266,7 @@ fn optimize_scalar( let subqry_plan = subqry_plan .aggregate(group_by, aggr.aggr_expr.clone())? .project(proj)? - .alias(&subqry_alias)? + .alias(subqry_alias.clone())? .build()?; // qualify the join columns for outside the subquery From 3ced958e3754c9c2e3afcfe9e45c67286880de22 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Sat, 15 Apr 2023 14:23:30 -0500 Subject: [PATCH 2/4] proto changes --- datafusion/expr/src/logical_plan/plan.rs | 2 +- datafusion/proto/proto/datafusion.proto | 3 ++- datafusion/proto/src/generated/pbjson.rs | 10 +++++----- datafusion/proto/src/generated/prost.rs | 4 ++-- datafusion/proto/src/logical_plan/mod.rs | 13 ++++++++----- 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 662c403f024a..8833b702764a 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -1902,7 +1902,7 @@ mod tests { use crate::{col, exists, in_subquery, lit}; use arrow::datatypes::{DataType, Field, Schema}; use datafusion_common::tree_node::TreeNodeVisitor; - use datafusion_common::DFSchema; + use datafusion_common::{DFSchema, TableReference}; use std::collections::HashMap; fn employee_schema() -> Schema { diff --git a/datafusion/proto/proto/datafusion.proto b/datafusion/proto/proto/datafusion.proto index 49cf9c980660..0b0a7f393187 100644 --- a/datafusion/proto/proto/datafusion.proto +++ b/datafusion/proto/proto/datafusion.proto @@ -294,8 +294,9 @@ message SelectionExecNode { } message SubqueryAliasNode { + reserved 2; // Was string alias LogicalPlanNode input = 1; - string alias = 2; + OwnedTableReference alias = 3; } // logical expressions diff --git a/datafusion/proto/src/generated/pbjson.rs b/datafusion/proto/src/generated/pbjson.rs index 3dca53e7b5ef..3848fdc3193e 100644 --- a/datafusion/proto/src/generated/pbjson.rs +++ b/datafusion/proto/src/generated/pbjson.rs @@ -20113,15 +20113,15 @@ impl serde::Serialize for SubqueryAliasNode { if self.input.is_some() { len += 1; } - if !self.alias.is_empty() { + if self.alias.is_some() { len += 1; } let mut struct_ser = serializer.serialize_struct("datafusion.SubqueryAliasNode", len)?; if let Some(v) = self.input.as_ref() { struct_ser.serialize_field("input", v)?; } - if !self.alias.is_empty() { - struct_ser.serialize_field("alias", &self.alias)?; + if let Some(v) = self.alias.as_ref() { + struct_ser.serialize_field("alias", v)?; } struct_ser.end() } @@ -20197,13 +20197,13 @@ impl<'de> serde::Deserialize<'de> for SubqueryAliasNode { if alias__.is_some() { return Err(serde::de::Error::duplicate_field("alias")); } - alias__ = Some(map.next_value()?); + alias__ = map.next_value()?; } } } Ok(SubqueryAliasNode { input: input__, - alias: alias__.unwrap_or_default(), + alias: alias__, }) } } diff --git a/datafusion/proto/src/generated/prost.rs b/datafusion/proto/src/generated/prost.rs index 7e2d10ba8380..15d2960ede96 100644 --- a/datafusion/proto/src/generated/prost.rs +++ b/datafusion/proto/src/generated/prost.rs @@ -448,8 +448,8 @@ pub struct SelectionExecNode { pub struct SubqueryAliasNode { #[prost(message, optional, boxed, tag = "1")] pub input: ::core::option::Option<::prost::alloc::boxed::Box>, - #[prost(string, tag = "2")] - pub alias: ::prost::alloc::string::String, + #[prost(message, optional, tag = "3")] + pub alias: ::core::option::Option, } /// logical expressions #[allow(clippy::derive_partial_eq_without_eq)] diff --git a/datafusion/proto/src/logical_plan/mod.rs b/datafusion/proto/src/logical_plan/mod.rs index 5aa956c97e0a..0c58fada0a08 100644 --- a/datafusion/proto/src/logical_plan/mod.rs +++ b/datafusion/proto/src/logical_plan/mod.rs @@ -256,7 +256,8 @@ impl AsLogicalPlan for LogicalPlanNode { Some(a) => match a { protobuf::projection_node::OptionalAlias::Alias(alias) => { Ok(LogicalPlan::SubqueryAlias(SubqueryAlias::try_new( - new_proj, alias, + new_proj, + alias.clone(), )?)) } }, @@ -593,9 +594,11 @@ impl AsLogicalPlan for LogicalPlanNode { LogicalPlanType::SubqueryAlias(aliased_relation) => { let input: LogicalPlan = into_logical_plan!(aliased_relation.input, ctx, extension_codec)?; - LogicalPlanBuilder::from(input) - .alias(&aliased_relation.alias)? - .build() + let alias = from_owned_table_reference( + aliased_relation.alias.as_ref(), + "SubqueryAlias", + )?; + LogicalPlanBuilder::from(input).alias(alias)?.build() } LogicalPlanType::Limit(limit) => { let input: LogicalPlan = @@ -1069,7 +1072,7 @@ impl AsLogicalPlan for LogicalPlanNode { logical_plan_type: Some(LogicalPlanType::SubqueryAlias(Box::new( protobuf::SubqueryAliasNode { input: Some(Box::new(input)), - alias: alias.clone(), + alias: Some(alias.to_owned_reference().into()), }, ))), }) From fa41785987345f21ccb1237a45e1ff7aa62f8f50 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Sat, 15 Apr 2023 14:44:51 -0500 Subject: [PATCH 3/4] add sqllogictest --- .../tests/sqllogictests/test_files/ddl.slt | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/datafusion/core/tests/sqllogictests/test_files/ddl.slt b/datafusion/core/tests/sqllogictests/test_files/ddl.slt index d21d85267a04..59977feabe73 100644 --- a/datafusion/core/tests/sqllogictests/test_files/ddl.slt +++ b/datafusion/core/tests/sqllogictests/test_files/ddl.slt @@ -150,6 +150,26 @@ select * from "foo.bar.baz"; statement ok drop view "foo.bar.baz" +########## +# Query views in other schemas. +########## + +statement ok +CREATE SCHEMA foo_schema; + +# Should be able to create view in "foo_schema". +statement ok +CREATE VIEW foo_schema.bar AS VALUES(1,2); + +# And be able to query it. +query II +SELECT * FROM foo_schema.bar; +---- +1 2 + +# TODO: Drop schema for cleanup +# statement ok +# DROP SCHEMA foo_schema; ########## # Drop view error tests From bbf06d550e8fb880674360884a3de74a3f511c19 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Sun, 16 Apr 2023 10:40:44 -0500 Subject: [PATCH 4/4] Add tests --- .../tests/sqllogictests/test_files/ddl.slt | 21 +++++++++++++++++-- .../src/analyzer/inline_table_scan.rs | 6 +++--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/datafusion/core/tests/sqllogictests/test_files/ddl.slt b/datafusion/core/tests/sqllogictests/test_files/ddl.slt index 59977feabe73..addc20af9985 100644 --- a/datafusion/core/tests/sqllogictests/test_files/ddl.slt +++ b/datafusion/core/tests/sqllogictests/test_files/ddl.slt @@ -159,7 +159,7 @@ CREATE SCHEMA foo_schema; # Should be able to create view in "foo_schema". statement ok -CREATE VIEW foo_schema.bar AS VALUES(1,2); +CREATE VIEW foo_schema.bar AS (SELECT 1 as a, 2 as b); # And be able to query it. query II @@ -167,7 +167,24 @@ SELECT * FROM foo_schema.bar; ---- 1 2 -# TODO: Drop schema for cleanup +# Make sure we can query individual columns with various qualifications. + +query I +SELECT a FROM foo_schema.bar; +---- +1 + +query I +SELECT bar.a FROM foo_schema.bar; +---- +1 + +query I +SELECT foo_schema.bar.a FROM foo_schema.bar; +---- +1 + +# TODO: Drop schema for cleanup, see #6027 # statement ok # DROP SCHEMA foo_schema; diff --git a/datafusion/optimizer/src/analyzer/inline_table_scan.rs b/datafusion/optimizer/src/analyzer/inline_table_scan.rs index 473fe33deccf..0b8c72b82264 100644 --- a/datafusion/optimizer/src/analyzer/inline_table_scan.rs +++ b/datafusion/optimizer/src/analyzer/inline_table_scan.rs @@ -64,9 +64,9 @@ fn analyze_internal(plan: LogicalPlan) -> Result> { let projection_exprs = generate_projection_expr(&projection, sub_plan)?; let plan = LogicalPlanBuilder::from(sub_plan.clone()) .project(projection_exprs)? - // This will ensure that the reference to the inlined table - // remains the same ensuring we don't have to change any of the - // parent nodes that reference this table. + // Ensures that the reference to the inlined table remains the + // same, meaning we don't have to change any of the parent nodes + // that reference this table. .alias(table_name)? .build()?; Transformed::Yes(plan)