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.

Sign up for GitHub

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

Jump to bottom

Added ability to parse STRUCT and MAP fields as well as nested arrays #966

Closed
wants to merge 1 commit into from
Closed

Added ability to parse STRUCT and MAP fields as well as nested arrays #966

wants to merge 1 commit into from

Conversation

scarman-db
Copy link

This PR has additions for parsing table columns with fields such as

CREATE TABLE t (
  m MAP<STRING, INT>,
  s STRUCT<field1: INT, field2: STRING, field3: INT>,
  a ARRAY<ARRAY<ARRAY<INT>>>
)

Additionally, this has the ability to parse back for some SQL dialects that use [] instead of the ARRAY<T> syntax adding the "SquareBracketedArrayinstead of justArray`

I also have a potential solution to the nested bracket issue, but I have no added it to Maps and structs although it should just work (I have to write tests a well) but I more was not happy with the manner in which I implemented this and I am looking for some suggestions on how to better structure this.

@scarman-db
Added ability to parse STRUCT and MAP fields as well as ARRAY with bo…
3f9de10 
…th bracket styles. Additionally, a speculative fix for double end greater than bracket
@alamb
Copy link
Contributor

alamb commented Sep 14, 2023

Thank you @scarman-db -- I'll check this out in the next few days

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6138442724

  • 59 of 79 (74.68%) changed or added relevant lines in 5 files are covered.
  • 40 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 86.922%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/data_type.rs 6 11 54.55%
src/ast/ddl.rs 0 5 0.0%
src/parser/mod.rs 42 52 80.77%
Files with Coverage Reduction New Missed Lines %
src/parser/mod.rs 40 82.13%
Totals Coverage Status
Change from base Build 6120887338: -0.2%
Covered Lines: 16317
Relevant Lines: 18772
💛 - Coveralls

alamb
alamb reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @scarman-db -- I am not quite sure what feedback you are looking for

This PR is failing CI

Also, I think there needs to be more doc comments to match the existing code style as ell as tests for the new Map, Struct and BracketArray structs

src/ast/data_type.rs
/// Enums
Enum(Vec<String>),
/// Set
Set(Vec<String>),
/// Map
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please provide a link to what SQL dialect supports this syntax?

src/ast/ddl.rs
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct StructField {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add docstrings that explain what this struct is for (with an example SQL snippet)

src/parser/mod.rs
@@ -257,6 +259,7 @@ pub struct Parser<'a> {
options: ParserOptions,
/// ensure the stack does not overflow by limiting recursion depth
recursion_counter: RecursionCounter,
max_depth: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document what this field is foe.

Also, could you please explain why you didn't use the counter in RecusionCounter?

@alamb alamb marked this pull request as draft October 24, 2023 10:39
@alamb
Copy link
Contributor

alamb commented Oct 24, 2023

Marking as draft as this PR is no longer waiting on feedback

@alamb alamb mentioned this pull request Oct 24, 2023
Merged
@alamb
Copy link
Contributor

alamb commented Oct 24, 2023

Similar PR here #1003

@scarman-db
Copy link
Author

I wanted feedback on the approach, but I looked at the other PR and it seems a bit more well thought out than mine. I'd suggest we can close this PR, but also for the other PR to add a test for the nesting arrays structs.

@alamb
Copy link
Contributor

alamb commented Oct 24, 2023

Thank you @scarman-db -- sorry for the delay in reviewing this PR

@alamb alamb closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scarman-db @alamb @coveralls