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

fix: infer Nullable/Array/LowCardinality with reflection #433

Merged
merged 2 commits into from
Oct 21, 2024
Merged

fix: infer Nullable/Array/LowCardinality with reflection #433

merged 2 commits into from
Oct 21, 2024

Conversation

serprex
Copy link
Member

@serprex serprex commented Oct 13, 2024 •
edited
Loading

helps re #152

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added

@serprex serprex requested review from genzgd and ernado October 13, 2024 04:59
@serprex
Copy link
Member Author

serprex commented Oct 13, 2024 •
edited
Loading

Not sure where CHANGELOG is

@serprex serprex force-pushed the infer-outer-reflection branch from 475ba40 to b89faa0 Compare October 13, 2024 05:09
@serprex serprex mentioned this pull request Oct 13, 2024
Draft
@serprex serprex force-pushed the infer-outer-reflection branch 2 times, most recently from e1d4a40 to 5eff914 Compare October 13, 2024 05:49
@serprex
Copy link
Member Author

serprex commented Oct 13, 2024

Unfortunately implementing Arrayable[[]T] for ColArr[T] fails due to invalid instantiation cycle. Also need to check if clickhouse has some special encoding for multidimensional arrays. Don't need/use them myself, so going to consider those out of scope

@ernado
Copy link
Collaborator

ernado commented Oct 15, 2024

Not sure where CHANGELOG is

There is no such one, template for PR looks outdated :(

@ernado
Copy link
Collaborator

ernado commented Oct 15, 2024

Please rebase

@serprex serprex force-pushed the infer-outer-reflection branch from 5eff914 to ba0a327 Compare October 16, 2024 12:14
@serprex
fix: infer Nullable/Array/LowCardinality with reflection
79d25b6 
helps re #152

couple fixes needed to get Array(Nullable(...)) & Nullable(DateTime64(...)) cases to work
@serprex
fix: typos in comments
7719a44
@serprex serprex force-pushed the infer-outer-reflection branch from ba0a327 to 7719a44 Compare October 17, 2024 19:21
@ernado ernado merged commit 0ae6dfb into main Oct 21, 2024
25 checks passed
@ernado ernado deleted the infer-outer-reflection branch October 21, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genzgd genzgd Awaiting requested review from genzgd

@ernado ernado Awaiting requested review from ernado

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

Successfully merging this pull request may close these issues.
2 participants
@serprex @ernado