ARRAY[] function #1264

kivikakk · 2017-10-20T05:32:53Z

TODO:

Proper testing.
Put things where they belong. It should be somewhere pg-specific.
Improve the interface. We shouldn't have to use different names for different argument counts.

kivikakk · 2017-10-20T06:06:58Z

diesel/diesel_tests/tests/expressions/mod.rs

Line 364 in a0fce39

let value = select(array2::<Int4, _, Int4, _ ,Int4>(1, 2))

This is an eyesore, but I'm not sure how to convince the compiler to infer r as equal(ish?!) to the $arg_struct_name idents …

kivikakk · 2017-10-20T06:07:36Z

I'd be curious on anyone's comments before I proceed with trying to clean this up!

sgrif · 2017-10-22T16:53:38Z

I don't like this approach. I'd much rather we provide a single function called array which takes a tuple.

kivikakk · 2017-10-26T07:02:12Z

I absolutely agree! Hence asking for comments. :) I'll work on that shortly.

sgrif · 2017-10-26T16:27:29Z

So I'd roughly take this approach:

// Not super stoked on this name
pub trait IntoSingleTypeExpressionList<ST> {
    type Expression;

    fn into_single_type_expression_list(self) -> Self::Expression;
}

// This would go in types/impls/tuples.rs
impl<$($T,)+ ST> IntoSingleTypeExpressionList<ST> for ($($T,)+)
where
    $($T: AsExpression<ST>,)+
{
    type Output = ($(AsExprOf<$T, ST>,)+);

    fn into_single_type_expression_list(self) -> Self::Expression {
        ($(self.$idx.into_sql::<ST>(),)+)
    }
}

pub struct Array<T, ST> {
    elements: T,
    _marker: PhantomData<ST>,
}

// Order here is important, it's likely people will sometimes need to specify the sql type, but infer the actual argument being passed.
pub fn array<ST, T>(elements: T) -> Array<T::Expression, ST>
where
    T: IntoSingleTypeExpressionList<ST>,
{
    Array {
        elements: elements.into_single_type_expression_list(),
        _marker: PhantomData,
    }
}

impl<T, ST> Expression for Array<T, ST>
where
    T: Expression,
{
    type SqlType = types::Array<ST>;
}

It's worth noting that a single element array literal would look a little funky, as it'd be passed like: array((foo,)).

kivikakk · 2017-11-16T06:35:55Z

This is now looking better. Thanks for your impeccable guidance there; it slotted right in with few changes.

Does the current positioning of the traits and structs for supporting this look OK?

weiznich · 2017-11-16T09:50:40Z

diesel/src/pg/expression/array.rs

+    _marker: PhantomData<ST>,
+}
+
+pub fn array<ST, T>(elements: T) -> Array<T::Expression, ST>


It would be great to add some small documentation with an example to this function.

Good point, thank you! Added in bb35442.

weiznich · 2017-11-16T18:04:50Z

diesel/src/pg/expression/array.rs

+/// #     }
+/// # }
+/// #
+/// # #[cfg(feature = "postgres")]


Because this lives in diesel/src/pg I think we do not need the #[cfg(feature = "postgres")] annotation.

(Same for the empty main function for the not-pg case)

weiznich · 2017-11-16T18:09:18Z

diesel/src/expression/mod.rs

@@ -300,3 +300,9 @@ impl<QS, ST, DB> QueryId for BoxableExpression<QS, DB, SqlType = ST> {

    const HAS_STATIC_QUERY_ID: bool = false;
 }
+
+pub trait IntoSingleTypeExpressionList<ST> {


I think this trait should also live in diesel/src/pg/expression/array.rs because it is Postgres specific.

sgrif · 2017-11-16T22:58:11Z

diesel/src/pg/expression/array.rs

+where
+    T: Expression,
+{
+    type SqlType = ::pg::types::sql_types::Array<ST>;


This can just be types::Array

Not in this context? types isn't imported here.

Or ::types::Array. Is there a reason for us to not import it?

Yeah, good point. Fixed.

sgrif · 2017-11-16T22:58:37Z

diesel/src/pg/expression/array.rs

+{
+    fn walk_ast(&self, mut out: AstPass<DB>) -> ::result::QueryResult<()> {
+        out.push_sql("ARRAY[");
+        QueryFragment::walk_ast(&(&self.elements,), out.reborrow())?;


Why the tuple?

Good question! Dropping it.

sgrif · 2017-11-16T22:58:59Z

diesel/src/pg/expression/array.rs

+{
+}
+
+impl<T, ST> ::expression::NonAggregate for Array<T, ST>


Can we import this?

sgrif · 2017-11-16T22:59:25Z

diesel/src/pg/expression/mod.rs

@@ -1,5 +1,7 @@
 #[doc(hidden)]
 pub mod array_comparison;
+#[doc(hidden)]
+pub mod array;


Does this need to be public?

Now that IntoSingleTypeExpressionList is in that module, yes.

Can we have a #[doc(hidden)] reexport of this from somewhere public and make the module private?

I'm not exactly sure what shape this should take, having tried a few ways. I've tried something in 99783c0, but it feels janky and wrong. Some documentation added too.

sgrif · 2017-11-17T16:26:22Z

This needs some compile-fail tests as well, probably checking that: only usable with pg, expressions have to be the right type, expressions have to have the same type

kivikakk · 2017-11-19T03:30:36Z

Good call! Done.

sgrif · 2017-11-21T22:17:26Z

diesel/src/pg/expression/array.rs

+}
+
+use query_builder::{AstPass, QueryFragment};
+use backend::Backend;


Can we keep all the use statements at the top?

sgrif · 2018-01-03T22:53:51Z

diesel/src/pg/expression/array.rs

+}
+
+#[derive(Debug)]
+pub struct Array<T, ST> {


What do you think about naming this ArrayLiteral so it's clear what the distinction from types::Array is?

sgrif · 2018-01-03T22:53:59Z

diesel/src/pg/expression/array.rs

+    fn into_single_type_expression_list(self) -> Self::Expression;
+}
+
+#[derive(Debug)]


This should implement Clone and Copy.

sgrif · 2018-01-03T22:54:19Z

diesel/src/pg/expression/array.rs

+    _marker: PhantomData<ST>,
+}
+
+/// Creates an `ARRAY[...]` expression.  The argument should be a tuple of


The first sentence should be its own paragraph.

sgrif · 2018-01-03T22:55:15Z

diesel/src/pg/expression/array.rs

+/// // An array is returned as a Vec.
+/// assert_eq!(Ok(vec![1, 2]), ints);
+///
+/// // The type of `id` is known so we don't have to specify the SQL type here.


This doc comment mentions type inference more than anything else. Do you think this is likely to be used in contexts where the type can't be inferred enough to warrant this much attention?

I noticed behaviour where it wasn't, but I think that was in a previous iteration. I'll drop the excess.

sgrif · 2018-01-03T22:55:52Z

diesel/src/pg/expression/array.rs

+    }
+}
+
+impl_query_id!(Array<T, ST>);


Note: If #1391 is merged before this, this needs to get updated.

Understood.

sgrif · 2018-01-03T22:56:55Z

diesel/src/pg/expression/mod.rs

@@ -1,5 +1,7 @@
 #[doc(hidden)]
 pub mod array_comparison;
+#[doc(hidden)]
+pub mod array;


Can we have a #[doc(hidden)] reexport of this from somewhere public and make the module private?

sgrif · 2018-01-03T22:57:22Z

diesel/src/types/impls/tuples.rs

@@ -250,6 +250,18 @@ macro_rules! tuple_impls {
                    ($($T,)+ next)
                }
            }
+
+            #[cfg(feature = "postgres")]
+            impl<$($T,)+ ST> $crate::pg::expression::array::IntoSingleTypeExpressionList<ST> for ($($T,)+) where


There's no need to use fully qualified paths vs just useing the trait here. This macro doesn't get called from user code.

sgrif · 2018-01-03T22:57:43Z

diesel_compile_tests/tests/compile-fail/array_expressions_must_be_correct_type.rs

+    let connection = PgConnection::establish("").unwrap();
+    select(array((1, 3))).get_result::<Vec<i32>>(&connection);
+    select(array((1f64, 3f64))).get_result::<Vec<i32>>(&connection);
+    //~^ ERROR E0277


Can we be specific on at least one of these? (Also why are there so many?)

They're like this:

unexpected errors (from JSON output): [ Error { line_num: 10, kind: Some( Error ), msg: "10:5: 10:11: the trait bound `f64: diesel::SelectableExpression<()>` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:33: 10:43: the trait bound `f64: diesel::expression::NonAggregate` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:33: 10:43: the trait bound `f64: diesel::SelectableExpression<()>` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:33: 10:43: the trait bound `f64: diesel::query_builder::QueryId` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:12: 10:17: the trait bound `f64: diesel::Expression` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:33: 10:43: the trait bound `f64: diesel::query_builder::QueryFragment<_>` is not satisfied [E0277]" }, Error { line_num: 10, kind: Some( Error ), msg: "10:12: 10:31: the trait bound `f64: diesel::Expression` is not satisfied [E0277]" } ]

I'll add some specifics.

Ughhhhh actually add none of those. Seriously Rust. Just fucking say "f64: AsExpression<Integer> is not satisfied" these errors are ridiculous.

sgrif · 2018-01-04T00:08:58Z

Thank you for all your hard work, this is an excellent PR! <3

sgrif · 2018-01-12T19:42:36Z

I manually rebased, and made additional changes. This was merged in 8fb0429

kivikakk mentioned this pull request Oct 20, 2017

Array operators TheConner/diesel_ltree#2

Closed

weiznich reviewed Nov 16, 2017

View reviewed changes

sgrif reviewed Nov 16, 2017

View reviewed changes

sgrif reviewed Nov 21, 2017

View reviewed changes

kivikakk added 13 commits December 3, 2017 20:15

jank

af8f585

Generic array functions

03568ba

start on some tests

140bb5d

more tests. type inference lacking.

a36f00b

less rubbish

0ad906d

Use IntoSingleTypeExpressionList for array

099d0f8

move this into pg-only land

f7c86f7

Add documentation to array

0fbd6e9

Don't need feature flags under pg tree

14a357b

Move IntoSingleTypeExpressionList<ST> into pg tree

6d2f022

cleanup

a7b331c

Add compile-fail tests for arrays

5b29a06

fix up this monstrosity

e51601d

sgrif added this to the 1.1 milestone Dec 3, 2017

sgrif requested changes Jan 3, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into array-fn

f78a8ce

kivikakk added 2 commits January 4, 2018 10:34

cleaning up

5255f7c

rearrange and document

99783c0

kivikakk added 2 commits January 4, 2018 11:50

correct lint errors

7232956

correct fmt

8b76aa8

sgrif closed this Jan 12, 2018

sgrif added a commit that referenced this pull request Jan 12, 2018

CHANGELOG for #1264 [ci skip]

efb2a55

kivikakk deleted the array-fn branch January 12, 2018 22:05

ARRAY[] function #1264

ARRAY[] function #1264

Conversation

kivikakk commented Oct 20, 2017 • edited

kivikakk commented Oct 20, 2017 • edited

kivikakk commented Oct 20, 2017

sgrif commented Oct 22, 2017 • edited

kivikakk commented Oct 26, 2017

sgrif commented Oct 26, 2017

kivikakk commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Nov 17, 2017

kivikakk commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Jan 4, 2018

sgrif commented Jan 12, 2018

kivikakk commented Oct 20, 2017 •

edited

kivikakk commented Oct 20, 2017 •

edited

sgrif commented Oct 22, 2017 •

edited

sgrif Jan 3, 2018 •

edited