feat: support `INSERT INTO [TABLE] FUNCTION` of Clickhouse by byte-sourcerer · Pull Request #1633 · apache/datafusion-sqlparser-rs

byte-sourcerer · 2025-01-02T03:28:08Z

Support INSERT INTO [TABLE] FUNCTION of Clickhouse.

iffyio

iffyio · 2025-01-05T11:51:33Z

src/ast/dml.rs

    /// TABLE
-    #[cfg_attr(feature = "visitor", visit(with = "visit_relation"))]
-    pub table_name: ObjectName,
+    pub table_object: TableObject,


Suggested change

pub table_object: TableObject,

pub table: TableObject,

There is already a field named table in this struct...

Ah right, I didn't notice the existing table property. Looking at the description of that property it feels its naming isn't ideal, I'm thinking as a result we could take the opportunity to rename that one to e.g. has_table_keyword to free up the current one to be called table?

iffyio · 2025-01-05T11:53:36Z

src/ast/mod.rs

+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum TableObject {


Could we include a doc comment mentionign this is the target of an insert table statement? And then for the variants that it can either be a tablename or function with example SQL? That would make it easy for folks to get an overview of the struct without having to dig into the code or test out manually

iffyio · 2025-01-05T11:55:28Z

src/parser/mod.rs


+    pub fn parse_table_object(
+        &mut self,
+        in_table_clause: bool,


Is the in_table_clause flag needed? I imagine it'll always be false

iffyio · 2025-01-05T11:56:07Z

src/parser/mod.rs

        }
    }

+    pub fn parse_table_object(


Can we add a doc comment for this given its a public method?

iffyio · 2025-01-05T11:57:50Z

src/parser/mod.rs

+        &mut self,
+        in_table_clause: bool,
+    ) -> Result<TableObject, ParserError> {
+        if dialect_of!(self is ClickHouseDialect) && self.parse_keyword(Keyword::FUNCTION) {


Can we use a dialect method for this? e.g.

if self.dialect.supports_function_table_objects() && self.parse_keyword(keyword::FUNCTION)

Refactored.

iffyio · 2025-01-05T11:59:14Z

src/parser/mod.rs

+    pub fn parse_table_function(&mut self) -> Result<Function, ParserError> {
+        let fn_name = self.parse_object_name(false)?;
+        self.parse_function_call(fn_name)
+    }


I'm wondering if its worth having this as a standalone method? It looks like it can be inlined given its only 2 LOC?

iffyio · 2025-01-05T11:59:57Z

tests/sqlparser_clickhouse.rs


+#[test]
+fn parse_insert_into_function() {
+    clickhouse().verified_stmt(r#"INSERT INTO TABLE FUNCTION remote('localhost', default.simple_table) VALUES (100, 'inserted via remote()')"#);


Can we add a similar test case without the TABLE keyword?

byte-sourcerer · 2025-01-08T13:42:53Z

@iffyio Thank you for your time. I have revised the PR according to your comments and hope you can review it.

iffyio · 2025-01-08T17:36:35Z

src/ast/dml.rs

    /// TABLE
-    #[cfg_attr(feature = "visitor", visit(with = "visit_relation"))]
-    pub table_name: ObjectName,
+    pub table_object: TableObject,


Ah right, I didn't notice the existing table property. Looking at the description of that property it feels its naming isn't ideal, I'm thinking as a result we could take the opportunity to rename that one to e.g. has_table_keyword to free up the current one to be called table?

src/ast/mod.rs

src/dialect/mod.rs

byte-sourcerer · 2025-01-09T02:57:19Z

Hi @iffyio, I have revised the PR. Could you please review it again?

iffyio

LGTM! Thanks @byte-sourcerer!
cc @alamb

iffyio · 2025-01-09T23:33:59Z

@byte-sourcerer could you take a look when you get the chance to resolve the conflicts on the branch

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

byte-sourcerer · 2025-01-10T02:56:08Z

@byte-sourcerer could you take a look when you get the chance to resolve the conflicts on the branch

Resolved.

Co-authored-by: Kermit <chenjiawei1@xiaohongshu.com> Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

byte-sourcerer force-pushed the insert-into-function branch from d4ac9e2 to 123d574 Compare January 2, 2025 03:36

iffyio reviewed Jan 5, 2025

View reviewed changes

byte-sourcerer force-pushed the insert-into-function branch from 9e648e8 to 229d7db Compare January 8, 2025 13:40

iffyio reviewed Jan 8, 2025

View reviewed changes

iffyio approved these changes Jan 9, 2025

View reviewed changes

byte-sourcerer and others added 12 commits January 10, 2025 10:44

feat: support INSERT INTO FUNCTION

c33c8f6

resolve comments

55f2230

resolve comments again

d540a27

fix rebasing conflicts

486f3a8

refactor: rename table/table_object

bc8757d

Update comment of TableObject src/ast/mod.rs

8ea3bd0

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

Rename supports_table_function_in_insertion in src/dialect/mod.rs

7b375ac

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

fix: renaming

51187d6

Update comment of TableObject::TableFunction in src/ast/mod.rs

00bf0b2

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

Update comment of TableObject::TableName src/ast/mod.rs

3711a55

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

Update src/ast/mod.rs

6a19804

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

resolving rebasing conflicts

6137566

byte-sourcerer force-pushed the insert-into-function branch from ed5f542 to 6137566 Compare January 10, 2025 02:54

iffyio merged commit b09514e into apache:main Jan 10, 2025
9 checks passed

byte-sourcerer deleted the insert-into-function branch January 11, 2025 02:41

ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Apr 10, 2025

feat: support INSERT INTO [TABLE] FUNCTION of Clickhouse (apache#1633)

bbece90

Co-authored-by: Kermit <chenjiawei1@xiaohongshu.com> Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

Conversation

byte-sourcerer commented Jan 2, 2025

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byte-sourcerer commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

byte-sourcerer commented Jan 9, 2025

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

iffyio commented Jan 9, 2025

Uh oh!

byte-sourcerer commented Jan 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

byte-sourcerer commented Jan 8, 2025 •

edited

Loading