Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 40 commits into from
Dec 1, 2022

Conversation

devgony
Copy link
Collaborator

@devgony devgony commented Nov 18, 2022

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>

Next

  • impl values for ast_builder
  • impl alias_as for ValuesNode

@devgony devgony added the improvement Improvements for existing features label Nov 18, 2022
@devgony devgony self-assigned this Nov 18, 2022
@coveralls
Copy link

coveralls commented Nov 18, 2022

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 Coverage Status
Change from base Build 3592201016: -0.02%
Covered Lines: 37460
Relevant Lines: 38019

💛 - Coveralls

@devgony devgony force-pushed the astBuilder-tableFactor branch 2 times, most recently from 6cdc2db to b9c370f Compare November 19, 2022 13:04
@devgony devgony changed the title Support TableFactor in AstBuilder Support TableFactor:{Derived, Dictionary, Series} in AstBuilder Nov 19, 2022
@devgony devgony changed the title Support TableFactor:{Derived, Dictionary, Series} in AstBuilder Support TableFactor::{Derived, Dictionary, Series} in AstBuilder Nov 19, 2022
@devgony devgony marked this pull request as ready for review November 19, 2022 15:06
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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)?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@devgony devgony requested a review from ever0de November 20, 2022 03:08
Comment on lines 91 to 117
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),
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.


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

JoinNode::new(
self,
table_name.to_owned(),
Some(alias.to_owned()),
JoinOperatorType::Left,
)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@devgony devgony force-pushed the astBuilder-tableFactor branch 2 times, most recently from c3efb49 to d881363 Compare November 26, 2022 11:22
@devgony devgony requested a review from panarch November 26, 2022 11:23
@panarch panarch added enhancement New feature or request and removed improvement Improvements for existing features labels Nov 27, 2022
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@panarch panarch merged commit 8c88b9b into gluesql:main Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants