Enable map access for numbers, multiple nesting levels #356

Igosuki · 2021-09-16T07:34:52Z

This enables number based map access in the AST. I am unsure if this required a new type rather than just patching MapAccess but here goes.

alamb

Thanks @Igosuki !

Looks good to me. FYI @hntd187 as MapAccess seems to have been introduced in #235

Broadly speaking looks compatible with https://www.postgresql.org/docs/9.1/arrays.html too

alamb · 2021-09-16T20:55:15Z

tests/sqlparser_postgres.rs

@@ -669,6 +672,26 @@ fn parse_pg_regex_match_ops() {
    }
 }

+#[test]
+fn parse_map_access_expr() {
+    //let sql = "SELECT foo[0] as foozero, bar[\"baz\"] as barbaz FROM foos";


I wonder if you meant to leave this commented out

alamb · 2021-09-16T21:24:05Z

src/ast/mod.rs

@@ -251,7 +251,7 @@ pub enum Expr {
    },
    MapAccess {


I wonder if we can improve the docstrings here while we are modifying this file?

alamb · 2021-09-16T21:28:24Z

This PR also adds support for multiple levels of map access as well, right? Like not just ["foo"] also ["foo"]["bar"]

houqp

LGTM, thanks @Igosuki !

Igosuki · 2021-09-17T09:06:37Z

@alamb yeah the point is to enable infinite levels of nesting, I should probably add a test for that with arbitrary nesting of n

alamb · 2021-09-17T10:16:17Z

looks like there are some CI tests to clean up as well @Igosuki FYI

hntd187 · 2021-09-17T18:53:01Z

Looks good to me, I remember map access being something I was not entirely confident in for my initial work, but perhaps if one was able to extend it then it wasn't as bad as my own perception was of it.

Igosuki · 2021-09-18T14:06:46Z

Well, there will be occasions to expand upon this. For instance to support ranges, which are in the pg dialect (e.g. foo[1:2]). Also I think the display is wrong for map access regarding strings, because foo['bar'] and foo[bar] prints foo["'bar'"]
I added a test for string keys in map access as well to wrap it up.
Edit : ha but this breaks the hive dialect, which somehow doesn't print single quotes for a single quoted string

Igosuki · 2021-09-18T14:28:50Z

It breaks the hive test for map_access, since we don't differentiate between double and single quoted strings, I think this needs further fixing

…d strings for map access

Igosuki · 2021-09-24T09:45:12Z

Since map access belongs into the OLAP world and there is already the hive dialect, I went and enforced double quoted strings for map access in the display.

Igosuki · 2021-09-24T09:49:28Z

So now MapAccess is an infinite depth path access to nested values. I think allowing compound field selection within this should be a separate addition as it requires patching the tokenization so that '.' is allowed after brackets

coveralls · 2021-09-24T18:03:30Z

Pull Request Test Coverage Report for Build 1269308355

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

53 of 59 (89.83%) changed or added relevant lines in 4 files are covered.
293 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.07%) to 90.135%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/mod.rs	7	10	70.0%
src/parser.rs	14	17	82.35%

Files with Coverage Reduction	New Missed Lines	%
src/ast/operator.rs	2	93.02%
tests/sqlparser_common.rs	20	98.6%
src/parser.rs	134	85.86%
src/ast/mod.rs	137	70.82%

Totals
Change from base Build 1240364569:	0.07%
Covered Lines:	5820
Relevant Lines:	6457

💛 - Coveralls

alamb · 2021-09-24T18:02:15Z

src/ast/mod.rs

+                        .map(|k| {
+                            match k {
+                                k @ Value::Number(_, _) => format!("[{}]", k),
+                                Value::SingleQuotedString(s) => format!("[\"{}\"]", s),


It is odd to me that a SingleQuotedString is emitted with double quotes (") but it is consistent with the existing implementation

alamb · 2021-09-24T18:12:25Z

I think allowing compound field selection within this should be a separate addition as it requires patching the tokenization so that '.' is allowed after brackets

makes sense to me

alamb · 2021-09-24T18:18:38Z

FYI @Igosuki I fixed a CI failure with c9e3cfb

Igosuki · 2021-09-25T09:55:55Z

Thanks, now to enable this in data fusion Le ven. 24 sept. 2021 à 20:22, Andrew Lamb ***@***.***> a écrit :

…

Merged #356 <#356> into main. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#356 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADDFBWH2KRMP5NBKSK5EYDUDS6WBANCNFSM5EEDJNZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

houqp · 2021-09-25T20:22:23Z

Thank you for your work on this @Igosuki !

Igosuki · 2021-09-25T21:10:20Z

Thanks for merging Le sam. 25 sept. 2021 à 22:22, QP Hou ***@***.***> a écrit :

…

Thank you for your work on this @Igosuki <https://github.com/Igosuki> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#356 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADDFBWIW6OOXHUYOUX6NALUDYVQTANCNFSM5EEDJNZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

alamb · 2021-09-26T10:42:28Z

Thanks, now to enable this in data fusion

See apache/datafusion#1052

Igosuki added 2 commits September 15, 2021 21:15

enable integer keys for map access

18f8b84

enable map access for number keys

40de9d6

Igosuki mentioned this pull request Sep 16, 2021

Indexed field access for List apache/datafusion#1006

Merged

alamb approved these changes Sep 16, 2021

View reviewed changes

houqp approved these changes Sep 17, 2021

View reviewed changes

alamb changed the title ~~Enable map access for numbers~~ Enable map access for numbers, multiple nesting levels Sep 17, 2021

Add tests for string based map access

7d599de

Igosuki added 3 commits September 24, 2021 11:43

MapAccess: unbox single quoted strings to always display double quote…

90072c2

…d strings for map access

cargo fmt

a5bb345

cargo clippy

aba4192

alamb approved these changes Sep 24, 2021

View reviewed changes

Fix compilation with nost by avoiding format!

c9e3cfb

fix codestyle

6978991

alamb merged commit d498887 into apache:main Sep 24, 2021

This was referenced Sep 25, 2021

Update sqlparser-rs to 0.11 apache/datafusion#1051

Closed

Update sqlparser-rs to 0.11 apache/datafusion#1052

Merged

panarch mentioned this pull request Oct 21, 2021

Support Expr::MapAccess gluesql/gluesql#394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable map access for numbers, multiple nesting levels #356

Enable map access for numbers, multiple nesting levels #356

Igosuki commented Sep 16, 2021

alamb left a comment

alamb Sep 16, 2021

alamb Sep 16, 2021

alamb commented Sep 16, 2021

houqp left a comment

Igosuki commented Sep 17, 2021

alamb commented Sep 17, 2021

hntd187 commented Sep 17, 2021

Igosuki commented Sep 18, 2021 •

edited

Loading

Igosuki commented Sep 18, 2021

Igosuki commented Sep 24, 2021

Igosuki commented Sep 24, 2021 •

edited

Loading

coveralls commented Sep 24, 2021 •

edited

Loading

alamb Sep 24, 2021

alamb commented Sep 24, 2021

alamb commented Sep 24, 2021

Igosuki commented Sep 25, 2021 via email

houqp commented Sep 25, 2021

Igosuki commented Sep 25, 2021 via email

alamb commented Sep 26, 2021

Enable map access for numbers, multiple nesting levels #356

Enable map access for numbers, multiple nesting levels #356

Conversation

Igosuki commented Sep 16, 2021

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 16, 2021

Choose a reason for hiding this comment

alamb Sep 16, 2021

Choose a reason for hiding this comment

alamb commented Sep 16, 2021

houqp left a comment

Choose a reason for hiding this comment

Igosuki commented Sep 17, 2021

alamb commented Sep 17, 2021

hntd187 commented Sep 17, 2021

Igosuki commented Sep 18, 2021 • edited Loading

Igosuki commented Sep 18, 2021

Igosuki commented Sep 24, 2021

Igosuki commented Sep 24, 2021 • edited Loading

coveralls commented Sep 24, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1269308355

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

alamb Sep 24, 2021

Choose a reason for hiding this comment

alamb commented Sep 24, 2021

alamb commented Sep 24, 2021

Igosuki commented Sep 25, 2021 via email

houqp commented Sep 25, 2021

Igosuki commented Sep 25, 2021 via email

alamb commented Sep 26, 2021

Igosuki commented Sep 18, 2021 •

edited

Loading

Igosuki commented Sep 24, 2021 •

edited

Loading

coveralls commented Sep 24, 2021 •

edited

Loading