-
Notifications
You must be signed in to change notification settings - Fork 810
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(postgresql): Add support for PostgreSQL multi-dimensional arrays #2338
feat(postgresql): Add support for PostgreSQL multi-dimensional arrays #2338
Conversation
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…e/pg-multi-dim-arrays
…73/feature-pg-multi-dim-arrays
…e/pg-multi-dim-arrays
Thanks for your work and patience on this PR. I'd like to get it merged before the 1.20 release next week. I think all that's needed is to regenerate the test output and resolve conflicts (if there are any) one more time. |
@andrewmbenton thank you for getting this PR some attention. I appreciate it. It likely needs another code review in addition to the CI checks given how things have changed since the initial PR was opened. |
I'm re-running our automated CI checks now, and will do one more code review for sure. |
So there are a couple of things I would change, but I don't want to make you put in more effort on this so I'm happy to take it over the finish line if you don't have time. At a high level, I would name this new parameter The one failing test is not a big deal. I think you just need to add stderr.txt with the correct string to the directory (can copy from main branch). There is an issue with some endtoend test output changing though. In particular I noticed that the I'm going to attempt to fix the broken test on your branch, and then I can dig in to see if I can discover what's changing the |
If you're able to finish up this PR, that'd be great! Otherwise, I'll have time next weekend to work on it. Given the target of 1.20, I know that doesn't line up well timeline-wise. I agree on the ArrayDims name change, it's definitely clearer. If you have any more questions on the implementation, let me know and I'll see if I can get an answer to you during the week. Thanks again |
This PR adds support for generating model fields from multi-dimensional postgresql array columns. The approach here is to pass around an additional column attribute array_bounds representing the dimensions of the postgres array column. When generating code, the array_bounds is checked along is_array to determine the dimensions of the resulting struct field. Would be happy to hear alternative approaches or solutions here as well. More info: Postgres Arrays
fixes issue: #1494
Replaces #1651 #2308 #2309