-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Map access supports constant-resolvable expressions #14712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Lordworms -- this looks great and very close
I think it just needs a few more negative tests
cc @goldmedal
@@ -592,6 +592,24 @@ select map_extract(column1, 1), map_extract(column1, 5), map_extract(column1, 7) | |||
[NULL] [NULL] [[1, NULL, 3]] | |||
[NULL] [NULL] [NULL] | |||
|
|||
query ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also please add some error tests for out of bounds access. For example, column[0]
, column[-1]
and column[1000]
to test the boundary condidtions?
Also, it seems like the code in this PR supports queries like
select column1[column2]
where column2 is an array of integer as well
Can you also add a test for that syntax as well?
.slice(start, end - start) | ||
.iter() | ||
.enumerate() | ||
.find(|(_, t)| t.unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this panic is never called, but then I see you just moved the code
Also FYI @findepi |
acd81e7
to
92174c1
Compare
fix clippy fix clippy fix clippy
92174c1
to
1265bba
Compare
thanks for your PR! What are "constant-resolvable expressions"? |
I should change the title, primitive and part of the nested types(array, struct, list) are supported now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Lordworms and @findepi -- looks good to me
For soem reason this PR caused a logical / build conflict |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?