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.

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

#841: Support parsing a field as an SQL array. #854

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Conversation

jtv
Copy link
Owner

@jtv jtv commented Jun 23, 2024

Adds a field member function, as_sql_array(), which reads an SQL array value into a pqxx::array.

test/unit/test_array.cxx Fixed Show fixed Hide fixed
@jtv jtv marked this pull request as draft June 23, 2024 23:35
@jtv jtv force-pushed the 841-field-as-array branch 2 times, most recently from 050af51 to f517fa8 Compare June 26, 2024 22:36
test/unit/test_array.cxx Dismissed Show dismissed Hide dismissed
/// Parse the field as an SQL array.
/** Call the parser to retrieve values (and structure) from the array.
*
* Make sure the @ref result object stays alive until parsing is finished. If
* you keep the @ref row of `field` object alive, it will keep the @ref
* result object alive as well.
*/
[[deprecated(
Copy link
Contributor

@fallenworld1 fallenworld1 Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this deprecation. Looks like pqxx::array doesn't allow move and extraction to an external structure, which could be useful. Otherwise looks great

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I suppose we could consider supporting moves, if we find a good use for them. But when are they really worthwhile? Perhaps if the array elements were of a custom time which in turn contained an array, or something like that.

Same consideration for extraction to an external structure: when is that helpful? You can copy elements right now. Or just keep the pqxx::array object as the basic storage for that data.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fallenworld1 for now if you don't mind I think I'll just merge this, and then make any incremental improvements as needed.

@jtv jtv marked this pull request as ready for review June 30, 2024 21:31
@jtv
Copy link
Owner Author

jtv commented Jun 30, 2024

It's great to have several members of the community look into this. This level of involvement can save us from design mistakes that may creep in if I just do things on my own.

We've got some suggestions for future improvements — notably on getting the data out and into some other data structure more efficiently. I think the easiest thing we could do about that is to support non-const forms of array indexing, where you can move your elements out instead of copying them. What I particularly like about that idea is that we can add it in as an afterthought without affecting the design.

@jtv jtv merged commit 840a13a into master Jul 5, 2024
7 checks passed
@jtv jtv deleted the 841-field-as-array branch July 5, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants