Fix querying and defining table / view names with period #4530

alamb · 2022-12-06T19:01:37Z

Which issue does this PR close?

Closes #4513
Closes #4531

This is really not that large of a PR

This looks like a massive change but a substantial amount of it is generated protobuf code

Rationale for this change

See more details on #4513

TLDR is that this SQL:

❯ select * from "foo.bar";
Plan("failed to resolve schema: foo")

Should not be treating "foo" as a schema name

The issue is that the SQL planner was flattening multi part identifiers like ['"foo"', 'bar'] into Strings like '"foo".bar' and then the strings were being parsed by https://github.com/apache/arrow-datafusion/blob/740a4fa2c6ba4b85875a433bb86e5b00435a5969/datafusion/common/src/table_reference.rs#L97 which does not parse them properly (instead it simply splits them on . rather than handling " properly)

What changes are included in this PR?

Changes:

Add OwnedTableReference, and change the planner and relevant DML plan nodes to use that rather than a String
Add docstrings in impl<'a> From<&'a str> for TableReference<'a> { to document its limitations.
Added protobuf serialization support
Add new .slt tests for same

The correct way to parse strings to references is to run through the SQL parser and our planner, but this API seems to be a fairly large part of the public API. I filed a follow on ticket to track properly supporting name resolution via the API: #4532

Are these changes tested?

Yes (both extensively with existing tests as well as new tests)

Are there any user-facing changes?

Better support for names with periods

alamb · 2022-12-06T19:39:32Z

datafusion/common/src/dfschema.rs

@@ -206,7 +206,7 @@ impl DFSchema {
                (Some(qq), None) => {
                    // the original field may now be aliased with a name that matches the
                    // original qualified name
-                    let table_ref: TableReference = field.name().as_str().into();
+                    let table_ref = TableReference::parse_str(field.name().as_str());


I am not sure if this is correct as it potentially splits on "." -- but I chose to keep the existing behavior

alamb · 2022-12-06T19:41:37Z

datafusion/expr/src/logical_plan/plan.rs

@@ -1058,7 +1060,7 @@ pub struct CreateCatalogSchema {
 #[derive(Clone)]
 pub struct DropTable {
    /// The table name
-    pub name: String,
+    pub name: OwnedTableReference,


This is the key change -- previously in DataFusion relation (table, view, etc) were stored as String which was then "parsed" (really split on .). This PR changes the code so the original parsed multi-part TableReference is used instead

alamb · 2022-12-06T19:42:22Z

datafusion/proto/proto/datafusion.proto

@@ -915,6 +917,29 @@ message StringifiedPlan {
  string plan = 2;
 }

+message BareTableReference {


This is the corresponding serialization support for this feature.

alamb · 2022-12-06T19:43:32Z

datafusion/sql/src/utils.rs

@@ -542,3 +542,11 @@ pub(crate) fn normalize_ident(id: &Ident) -> String {
        None => id.value.to_ascii_lowercase(),
    }
 }
+
+// Normalize an owned identifier to a lowercase string unless the identifier is quoted.


I will remove the non-owned variant in a follow on PR -- but I was trying to minimize the size of this one

alamb · 2022-12-06T19:46:53Z

datafusion/sql/src/planner.rs

@@ -262,7 +264,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                };

                Ok(LogicalPlan::CreateMemoryTable(CreateMemoryTable {
-                    name: name.to_string(),
+                    name: object_name_to_table_reference(name)?,


Rather than encoding the name in a string, it is now encoded as a reference

alamb · 2022-12-06T19:51:26Z

datafusion/sql/src/planner.rs

+                    0 => Err(ParserError("Missing table name.".to_string()).into()),
+                    1 => object_name_to_table_reference(names.pop().unwrap()),
+                    _ => {
+                        Err(ParserError("Multiple objects not supported".to_string())


Previously if multiple objects were specified, the second was ignored: #4531

alamb · 2022-12-06T19:52:14Z

datafusion/sql/src/planner.rs

@@ -592,6 +602,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

        let schema = self.build_schema(columns)?;

+        // External tables do not support schemas at the moment, so the name is just a table name


the parser returns a String not an ObjectIdentifier, so I kept the behavior the same

alamb · 2022-12-06T19:52:57Z

datafusion/sql/src/planner.rs

-                                        }))
-                                    }
-                                }
+                    // only support "schema.table" type identifiers here


this has the same behavior as master but I think the logic is clearer

alamb · 2022-12-06T19:57:15Z

datafusion/core/tests/sqllogictests/src/insert/mod.rs

    // First, use sqlparser to get table name and insert values
-    let table_name;
+    let table_reference;


Update the test harness to use the new name handling as well

alamb · 2022-12-06T20:22:34Z

datafusion/core/tests/sqllogictests/test_files/ddl.slt

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+


I renamed insert.slt and appended to it.

Writing tests using sqllogictest is life changing . Thank you @mvanschellebeeck and @xudong963 for driving this work forward!

alamb · 2022-12-06T20:26:28Z

I am sorry to request a large PR code review @xudong963 and @andygrove but it is mostly tests and generated code.

cc @returnString in case you are around as you wrote the initial TableReference implementation

alamb · 2022-12-06T20:27:06Z

cc @NGA-TRAN in case you are interested

NGA-TRAN

The PR does what described

datafusion/common/src/table_reference.rs

NGA-TRAN · 2022-12-06T21:32:56Z

datafusion/core/tests/sqllogictests/test_files/ddl.slt

+##########
+
+# Verify that creating tables with periods in their name works
+# (note "foo.bar" is the table name, NOT table "bar" in schema "foo")


So the period in a table name only works if the name is in double quotes, right? Without double quotes. foo will be treated as a schema?
Is this "foo.bar.baz" a valid table name? can we add test for it?

Without double quotes. foo foo will be treated as a schema?

Yes

Is this "foo.bar.baz" a valid table name? can we add test for it?

It is a valid name. Added tests in c03cf2b -- thank you for the suggestion

xudong963 · 2022-12-07T07:05:08Z

I'll review it carefully in the evening.

Co-authored-by: Nga Tran <[email protected]>

…ith_periods

alamb · 2022-12-07T12:06:24Z

datafusion/core/tests/sqllogictests/test_files/insert.slt

-# KIND, either express or implied.  See the License for the
-# specific language governing permissions and limitations
-# under the License.
-


I renamed this file to ddl.slt and added some more tests, but the github UI doesn't seem to recognize that

xudong963

The ticket makes sense to me, thanks @alamb

FYI, databend processes the case in parser: catalog, database, and table are separated in parser

// Table name
Table {
    span: &'a [Token<'a>],
    catalog: Option<Identifier<'a>>,
    database: Option<Identifier<'a>>,
    table: Identifier<'a>,
    alias: Option<TableAlias<'a>>,
    travel_point: Option<TimeTravelPoint<'a>>,
},

/// Parse one two three idents seperated by a period, fulfilling from the right.
///
/// Example: `db.table.column`
#[allow(clippy::needless_lifetimes)]
pub fn peroid_separated_idents_1_to_3<'a>(
    i: Input<'a>,
) -> IResult<'a, (Option<Identifier>, Option<Identifier>, Identifier)> {
    map(
        rule! {
            #ident ~ ("." ~ #ident ~ ("." ~ #ident)?)?
        },
        |res| match res {
            (ident2, None) => (None, None, ident2),
            (ident1, Some((_, ident2, None))) => (None, Some(ident1), ident2),
            (ident0, Some((_, ident1, Some((_, ident2))))) => (Some(ident0), Some(ident1), ident2),
        },
    )(i)
}

alamb · 2022-12-07T17:55:11Z

FYI, databend processes the case in parser: catalog, database, and table are separated in parser

Makes sense @xudong963 -- this is basically what sqlparser-rs does as well (parses into Vec<Ident>). Thank you for the pointer.

alamb · 2022-12-07T17:55:42Z

I am merging this one in as I have a few PRs backed up behind it. Thanks again @NGA-TRAN and @xudong963 for your quick review.

ursabot · 2022-12-07T18:09:25Z

Benchmark runs are scheduled for baseline = 4c1f60c and contender = fbadebb. fbadebb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
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

Add tests for names with period

a5d5c45

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Dec 6, 2022

adjust docstrings

5169621

alamb commented Dec 6, 2022

View reviewed changes

alamb mentioned this pull request Dec 6, 2022

String --> TableReference parsing does not properly handle " and . #4532

Closed

alamb added the api change Changes the API exposed to users of the crate label Dec 6, 2022

alamb added 2 commits December 6, 2022 15:11

Improve docstrings

9c9c313

Add tests coverage

ad007bf

alamb commented Dec 6, 2022

View reviewed changes

alamb marked this pull request as ready for review December 6, 2022 20:22

alamb requested review from xudong963 and andygrove December 6, 2022 20:25

This was referenced Dec 6, 2022

Table names with periods are not handled correctly #4513

Closed

Minor: Avoid cloning as many Ident during SQL planning #4534

Merged

NGA-TRAN approved these changes Dec 6, 2022

View reviewed changes

alamb mentioned this pull request Dec 6, 2022

Move some create/drop tests to ddl.slt #4535

Merged

alamb and others added 3 commits December 7, 2022 07:00

Update datafusion/common/src/table_reference.rs

569979c

Co-authored-by: Nga Tran <[email protected]>

Add tests for creating tables with three periods

c03cf2b

Merge remote-tracking branch 'apache/master' into alamb/table_names_w…

e100bf2

…ith_periods

alamb commented Dec 7, 2022

View reviewed changes

xudong963 approved these changes Dec 7, 2022

View reviewed changes

alamb merged commit fbadebb into apache:master Dec 7, 2022

This was referenced Dec 7, 2022

feat: support prepare statement #4490

Merged

Confusing "Bare" in doesn't exist messages #4571

Closed

Remove Confusing "Bare" in does not exist messages #4572

Merged

alamb deleted the alamb/table_names_with_periods branch August 8, 2023 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix querying and defining table / view names with period #4530

Fix querying and defining table / view names with period #4530

alamb commented Dec 6, 2022 •

edited

Loading

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb Dec 6, 2022

alamb commented Dec 6, 2022

alamb commented Dec 6, 2022

NGA-TRAN left a comment

NGA-TRAN Dec 6, 2022

alamb Dec 7, 2022

xudong963 commented Dec 7, 2022

alamb Dec 7, 2022

xudong963 left a comment

alamb commented Dec 7, 2022

alamb commented Dec 7, 2022

ursabot commented Dec 7, 2022

		@@ -592,6 +602,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

		let schema = self.build_schema(columns)?;

		// External tables do not support schemas at the moment, so the name is just a table name

Fix querying and defining table / view names with period #4530

Fix querying and defining table / view names with period #4530

Conversation

alamb commented Dec 6, 2022 • edited Loading

Which issue does this PR close?

This is really not that large of a PR

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 6, 2022

alamb commented Dec 6, 2022

NGA-TRAN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Dec 7, 2022

Choose a reason for hiding this comment

xudong963 left a comment

Choose a reason for hiding this comment

alamb commented Dec 7, 2022

alamb commented Dec 7, 2022

ursabot commented Dec 7, 2022

alamb commented Dec 6, 2022 •

edited

Loading