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

issues-336 Init array support #390

Closed

Conversation

ikrivosheev
Copy link
Member

@ikrivosheev ikrivosheev commented Jul 19, 2022

PR Info

Depends:

Adds

  • Support Array

Todo:

  • Implement for all other types
  • Fix examples
  • Fix tests
  • Add documentation
  • Fix postgres driver
  • Fix sea-query-driver
  • Fix sea-query-binder
  • Remove todo from CommonQueryBuilder
  • Add methods for work with new Value items

@ikrivosheev
Copy link
Member Author

@Sytten @billy1624 @tyt2y3 This is Draft PR for add support Array type

@Sytten
Copy link
Contributor

Sytten commented Jul 19, 2022

Related to #336

This is basically proposition #2 and it looks well done (great job @ikrivosheev!).
I think it's a decent way to move forward pending a larger refactor as discussed with proposition #3.

@ikrivosheev
Copy link
Member Author

I have one question. How can we work with array of arrays?

@billy1624
Copy link
Member

@Sytten, have any experience working with array of arrays in PostgreSQL?

For me, I don't like the idea of having array datatype in database, let alone array of arrays lolll

We might want to leave array of arrays for later?

@billy1624
Copy link
Member

The code looks nice! @ikrivosheev please continue implementing array support :)
Thanks!!

@Sytten
Copy link
Contributor

Sytten commented Jul 20, 2022

@billy1624 I don't think array of arrays is a thing in postgres. I never saw it supported anywhere in any-case so no worries there.

I would say custom types are much more common (these days it is mostly for enums and composite keys) and arrays of custom types do happen once in a while. It is not as common now that json support is very well done, but it can happen and so it should be supported at some point.

@ikrivosheev
Copy link
Member Author

@Sytten I add support for array to sea-query-binder.

@ikrivosheev
Copy link
Member Author

ikrivosheev commented Jul 28, 2022

@Sytten hello! I have done some features:

  1. Add support for array in sea-query-binder
  2. Add support for array in sea-query-driver
  3. Add support for array in driver/postgres
  4. Tests are passed!

Can you review this part?

@Sytten
Copy link
Contributor

Sytten commented Jul 28, 2022

Will do tomorrow, pretty busy today. Ping me again if I forget

Copy link
Contributor

@Sytten Sytten left a comment

Choose a reason for hiding this comment

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

I don't have much to say at the moment, it is headed in the right direction for this support.
That being said I am not sure we really want to do this in the current state of things.
Personally I would move to a trait based system instead of the enum and I would consolidate the driver/binder mess before thinking about array support.

@billy1624
Copy link
Member

I don't have much to say at the moment, it is headed in the right direction for this support. That being said I am not sure we really want to do this in the current state of things. Personally I would move to a trait based system instead of the enum and I would consolidate the driver/binder mess before thinking about array support.

In the long run, I support adding a Value trait and redesigning the Value API from ground up. However, at the moment I think we can extend the functionality based on (current) Value enum.

Any comments? @tyt2y3

@ikrivosheev
Copy link
Member Author

I don't have much to say at the moment, it is headed in the right direction for this support. That being said I am not sure we really want to do this in the current state of things. Personally I would move to a trait based system instead of the enum and I would consolidate the driver/binder mess before thinking about array support.

In the long run, I support adding a Value trait and redesigning the Value API from ground up. However, at the moment I think we can extend the functionality based on (current) Value enum.

Any comments? @tyt2y3

I think about the current implementation and find it bad. A lot of code... Very hard to maintain.

@billy1624 if you want, I can create PR with my peace of code.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 1, 2022

It's quite a lot of boilerplate indeed, and I have the same concern over maintainability.

ValueTrait would help, but we should keep the primitive types in a enum, where i64 (and anything smaller than 8 bytes) should reside on the stack. For Object types (which we Boxed), we can probably use a fat pointer Box<&dyn ValueTrait> such that it allows users to impl that on their own.

What do you have in mind @ikrivosheev ?

@tyt2y3 tyt2y3 added this to the 0.27 milestone Aug 7, 2022
@billy1624 billy1624 removed this from the 0.27.x milestone Aug 24, 2022
@ikrivosheev ikrivosheev force-pushed the feature/issues-336_array_support branch from 5d9d694 to 79fab4a Compare October 9, 2022 14:52
@ikrivosheev
Copy link
Member Author

I close this PR. Better solution is: #467

@ikrivosheev ikrivosheev closed this Oct 9, 2022
@ikrivosheev ikrivosheev deleted the feature/issues-336_array_support branch October 10, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Query binders: Add support for postgres arrays
4 participants