Support `TableFactor::{Derived, Dictionary, Series}` in AstBuilder #1007

devgony · 2022-11-18T05:20:29Z

Goal

From now, we can use `Dictionary`, `SERIES(N)` and `Derived subquery` even in AstBuilder!

let actual = glue_objects().select().into();
let expected = "SELECT * FROM GLUE_OBJECTS";
test_query(actual, expected);

let actual = glue_tables().select().into();
let expected = "SELECT * FROM GLUE_TABLES";
test_query(actual, expected);

let actual = glue_indexes().select().into();
let expected = "SELECT * FROM GLUE_INDEXES";
test_query(actual, expected);

let actual = glue_table_columns().select().into();
let expected = "SELECT * FROM GLUE_TABLE_COLUMNS";
test_query(actual, expected);

let actual = series("1 + 2").select().into();
let expected = "SELECT * FROM SERIES(1 + 2)";
test_query(actual, expected);

let actual = table("Items").select().alias_as("Sub").select().into();
let expected = "SELECT * FROM (SELECT * FROM Items) AS Sub";
test_query(actual, expected);

Todo

implement Dictionary, Series with TableType
implement Derived with Box<SelectNode>

Pull Request Test Coverage Report for Build 3592779396

623 of 639 (97.5%) changed or added relevant lines in 17 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 98.53%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test-suite/src/ast_builder/alias_as.rs	232	248	93.55%

Files with Coverage Reduction	New Missed Lines	%
test-suite/src/tester/mod.rs	1	87.5%

Totals
Change from base Build 3592201016:	-0.02%
Covered Lines:	37460
Relevant Lines:	38019

💛 - Coveralls

core/src/ast_builder/select/root.rs

ever0de · 2022-11-19T16:46:20Z

core/src/ast_builder/table.rs

-impl TableNode {
-    pub fn alias_as(self, table_alias: &str) -> TableAliasNode {
+impl<'a> TableNode<'a> {
+    pub fn alias_as(self, table_alias: &'a str) -> TableAliasNode {


Suggested change

pub fn alias_as(self, table_alias: &'a str) -> TableAliasNode {

pub fn alias_as(self, table_alias: &str) -> TableAliasNode<'a> {

I think it is correct to declare this lifetime in TableAliasNode, not in str.

ever0de · 2022-11-19T16:57:31Z

core/src/translate/query.rs


            match object_name.as_str() {
                "SERIES" if args.is_some() => Ok(TableFactor::Series {
-                    alias: alias_or_name,
+                    alias: alias_or_name(alias, object_name),
                    size: translate_table_args(args)?,


It seems that there is unreachable code inside the translate_table_args closure, because the value of None for args will never go into translate_table_args.

panarch · 2022-11-24T10:20:39Z

core/src/ast_builder/mod.rs

+pub fn glue_objects<'a>() -> TableNode<'a> {
+    TableNode {
+        table_name: "GLUE_OBJECTS".to_owned(),
+        table_type: TableType::Dictionary(Dictionary::GlueObjects),
+    }
+}
+
+pub fn glue_tables<'a>() -> TableNode<'a> {
+    TableNode {
+        table_name: "GLUE_TABLES".to_owned(),
+        table_type: TableType::Dictionary(Dictionary::GlueTables),
+    }
+}
+
+pub fn glue_indexes<'a>() -> TableNode<'a> {
+    TableNode {
+        table_name: "GLUE_INDEXES".to_owned(),
+        table_type: TableType::Dictionary(Dictionary::GlueIndexes),
+    }
+}
+
+pub fn glue_table_columns<'a>() -> TableNode<'a> {
+    TableNode {
+        table_name: "GLUE_TABLE_COLUMNS".to_owned(),
+        table_type: TableType::Dictionary(Dictionary::GlueTableColumns),
+    }
+}


nit. except for series which takes Into<ExprNode<'a>>, all has static lifetimes.

e.g.

pub fn glue_table_columns() -> TableNode<'static> { .. }

this would be enough.

Now table sugar functions are more than a single one, then how about moving these including series into table.rs module?

panarch · 2022-11-24T10:36:46Z

core/src/ast_builder/select/root.rs

        JoinNode::new(
            self,
            table_name.to_owned(),
            Some(alias.to_owned()),
            JoinOperatorType::Left,
        )
    }
+
+    pub fn alias_as(self, table_alias: &'a str) -> TableAliasNode {


This is too cool feature! Could we have an usage example in test-suite/ ast_builder/select.rs?

SelectNode to QueryNode

panarch

Awesome 👍 👍 👍
Now we can chain select queries as many as we want.
All looks great! Thanks a lot 👍

devgony added the improvement Improvements for existing features label Nov 18, 2022

devgony requested review from panarch, ever0de and MRGRAVITY817 November 18, 2022 05:20

devgony self-assigned this Nov 18, 2022

devgony force-pushed the astBuilder-tableFactor branch 2 times, most recently from 6cdc2db to b9c370f Compare November 19, 2022 13:04

devgony commented Nov 19, 2022

View reviewed changes

core/src/ast_builder/select/root.rs Outdated Show resolved Hide resolved

devgony changed the title ~~Support TableFactor in AstBuilder~~ Support TableFactor:{Derived, Dictionary, Series} in AstBuilder Nov 19, 2022

devgony changed the title ~~Support TableFactor:{Derived, Dictionary, Series} in AstBuilder~~ Support TableFactor::{Derived, Dictionary, Series} in AstBuilder Nov 19, 2022

devgony marked this pull request as ready for review November 19, 2022 15:06

ever0de reviewed Nov 19, 2022

View reviewed changes

devgony requested a review from ever0de November 20, 2022 03:08

panarch requested changes Nov 24, 2022

View reviewed changes

devgony force-pushed the astBuilder-tableFactor branch 2 times, most recently from c3efb49 to d881363 Compare November 26, 2022 11:22

devgony requested a review from panarch November 26, 2022 11:23

panarch added enhancement New feature or request and removed improvement Improvements for existing features labels Nov 27, 2022

devgony added 9 commits December 1, 2022 12:49

test: add unit test GLUE_OBJECTS

86f693d

test: add more unit tests for GLUE_%

da114d8

feat: impl series, glue_*, with adding table_type, args to TableNode

2fed8a6

feat: impl TableType::Derived

f22c146

refactor: use tuple match

6ee2688

refactor: move args to Series(args)

0a43ecf

refactor: get subquery by build_query()

5766fde

refactor: remove redundant clone thanks to @ever0de

e15d31b

refactor: lighten alias_or_name with unwrap_or_else

3ca29bd

devgony added 26 commits December 1, 2022 12:49

fix: replace redundant lifetime to static thanks to @panarch

f5caceb

refactor: move table functions to table.rs thanks to @panarch

db5fa6d

feat: impl alias_as for OrderByNode, replace subquery type from

2ed766f

SelectNode to QueryNode

feat: impl alias_as for FilterNode

f9a8d92

feat: impl alias_as for ProjectNode

71e9318

feat: impl alias_as for JoinNode

63e9760

feat: imple alias_as for JoinConstraintNode

6c53b30

test: move cases to alias_as.rs

2eeb8bd

test: modify column name with verbose

fc2395f

feat: impl alias_as for GroupByNode

ca67a80

feat: impl alias_as for OffsetNode

80a4efb

feat: impl alias_as for LimitNode

d220235

feat: impl alias_as for OffsetLimitNode

c74182a

style: merge imports

631f5e5

feat: impl alias_as for HashJoinNode

0fb63f6

feat: impl alias_as for HavingNode

f216701

test: add unit test cases

f11c21c

refactor: impl alias_as for QueryNode to merge at once

9366f52

refactor: split TableNameNode and TableFactorNode

f2f4cf6

refactor: add alias_name to TableFactorNode

8914dc3

refactor: remove TableAliasNode

2891ea2

chore: remove comments

6f77d7b

chore: remove comments

4d65c4a

refactor: get table_factor manully

175ebb9

refactor: rename table.rs to table_name.rs

b938818

refactor: move fn table from table_factor to table_name

28d3049

devgony force-pushed the astBuilder-tableFactor branch from 1cdb4bb to 28d3049 Compare December 1, 2022 12:50

refactor: remove unused dml from table_factor

9e7f7f6

panarch approved these changes Dec 1, 2022

View reviewed changes

panarch merged commit 8c88b9b into gluesql:main Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `TableFactor::{Derived, Dictionary, Series}` in AstBuilder #1007

Support `TableFactor::{Derived, Dictionary, Series}` in AstBuilder #1007

devgony commented Nov 18, 2022 •

edited

Loading

coveralls commented Nov 18, 2022 •

edited

Loading

ever0de Nov 19, 2022

ever0de Nov 19, 2022

panarch Nov 24, 2022

panarch Nov 24, 2022

panarch left a comment

	pub fn alias_as(self, table_alias: &'a str) -> TableAliasNode {
	pub fn alias_as(self, table_alias: &str) -> TableAliasNode<'a> {

Support TableFactor::{Derived, Dictionary, Series} in AstBuilder #1007

Support TableFactor::{Derived, Dictionary, Series} in AstBuilder #1007

Conversation

devgony commented Nov 18, 2022 • edited Loading

Goal

From now, we can use Dictionary, SERIES(N) and Derived subquery even in AstBuilder!

Todo

Next

coveralls commented Nov 18, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3592779396

💛 - Coveralls

ever0de Nov 19, 2022

Choose a reason for hiding this comment

ever0de Nov 19, 2022

Choose a reason for hiding this comment

panarch Nov 24, 2022

Choose a reason for hiding this comment

panarch Nov 24, 2022

Choose a reason for hiding this comment

panarch left a comment

Choose a reason for hiding this comment

Support `TableFactor::{Derived, Dictionary, Series}` in AstBuilder #1007

Support `TableFactor::{Derived, Dictionary, Series}` in AstBuilder #1007

devgony commented Nov 18, 2022 •

edited

Loading

From now, we can use `Dictionary`, `SERIES(N)` and `Derived subquery` even in AstBuilder!

coveralls commented Nov 18, 2022 •

edited

Loading