Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: STRUCT and ARRAY support #318
feat: STRUCT and ARRAY support #318
Changes from 30 commits
52cee8c
a0b02f7
6bacc0d
1ec0f88
74aab64
c5653e2
a7f0b41
9df1804
0df1701
7aad07f
f10a571
ec31040
accf762
ef5f891
cce9dbb
290d955
b697df6
6a278b9
bc62a56
84426bd
587a0f7
e6f4adf
ffb5aa9
47fa14f
402bbbe
5bf07b4
e937167
8661f5b
b550aa1
af68a54
da43fd2
f04cac2
5af05bb
09866c6
054c227
1a79305
5e2ae32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we put this in a
_get_type_compiler
/_get_process
function? I don't see anywhere else we initializetype_compiler
, but I'd be more comfortable having this logic closer to the# We have to delay getting the type compiler, because of circular imports. :(
comment.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 refactored so this is combined and isolated in one place using a new, better named
_get_subtype_col_spec
function.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 assume
process
is able to handle nested arrays/structs?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.
yes
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.
Where does
_STRUCT__byname
come from? I'm assuming somewhere from SQLAlchemy, but I'm not getting any results when searching forbyname
.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.
Oh, I think I figured it out: https://docs.python.org/3/tutorial/classes.html#private-variables
Can we comment about this? I assume we have to do it because we know
self.expr.type
is aSTRUCT
, but it's notself
.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 refactored this to make name mangling more explicit and consistent, so I don't think comments are needed anymore. See if you agree. :)
I mainly use "private" variables, which aren't :), to avoid namespace conflicts when subclassing across responsibility boundaries. Arguably, explicit naming is better.
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'm a bit confused why
self.__byname
doesn't work in this case.Edit: I see now that it's part of the
Comparator
class. Still probably worth a similar comment to the one I recommend in_setup_getitem
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.
See my response on name mangling