Adding support for arrays #380

arjen-ag5 · 2024-09-04T06:28:15Z

Hello,

I'm working on support for arrays in go-jet. I was wondering if this is a feature that is considered in the long run and if it maybe merged into upstream in the long term. I'd like to show the progress I made so far and if it's any use trying to get it merged at all. If not I will not spend any effort in making it work with the different database types. Currently I only implemented postgres.

I can see from the Range support that is added earlier that the types from the pgtypes packages are being used. I took this into consideration for use as the SQL types in the generated table types. But the pgtypes.Array* structs are tailored to represent the internal representation of the postgresql array types. In constrast to the pq.Array* types which are much more user friendly, thats why I chose to use the latter.

…testcases on macOs

go-jet · 2024-09-07T10:54:03Z

Hi @arjen-ag5, thanks for starting the work on arrays.

go-jet · 2024-09-07T10:57:07Z

generator/template/model_template.go

+	if column.DataType.Dimensions > 1 {
+		return ""
+	}
+


Multi dimensional arrays are just arrays containing other arrays. In our case Array[Array[StringExpression]]. But it is fine with PR to go with single dimension arrays only.

You're right, but since we don't have a good model type yet, I haven't included this case yet

go-jet · 2024-09-07T10:59:45Z

generator/template/model_template.go

+	case "bigint[]":
+		return pq.Int64Array{}
+	case "text[]", "jsonb[]", "json[]":
+		return pq.StringArray{}


I'm fine using pq types for one dimensional arrays. Maybe pgtype can be used for multi dimensional arrays.
Other types as well, time, date, etc...

go-jet · 2024-09-07T11:03:19Z

generator/template/sql_builder_template.go

+	typeName := columnMetaData.DataType.Name
+	columnName := columnMetaData.Name
+
+	if columnMetaData.DataType.Kind == metadata.ArrayType {


Why not add a case switch for each of the array types, as it is already done for other types?

I added a new sqlArrayToColumnType to convert a type to an array type. The function switches on the type name without the [] suffix because you would have to add infinite brackets for multidimensional arrays.

When I think about it again it makes sense to separate arrays from sqlToColumnType. Since every postgres type can be element of array([]bool, []point, []timestamp, etc...), sql builder array type can be constructed with just sqlToColumnType:

columnType = sqlToColumnType(strings.TrimSuffix(typeName, "[]")) + "Array"

This can lead to incorrectly generated code, because this PR does not support DateArray for example. What's your opinion on this?

It can be supported with one line type DateArray Array[DateExpression]. For model type we can fallback to string if there is no pq date array type.

go-jet · 2024-09-07T11:04:49Z

generator/template/sql_builder_template.go

@@ -190,8 +222,7 @@ func getSqlBuilderColumnType(columnMetaData metadata.Column) string {
 	case "numrange":
 		return "NumericRange"
 	default:
-		fmt.Println("- [SQL Builder] Unsupported sql column '" + columnMetaData.Name + " " + columnMetaData.DataType.Name + "', using StringColumn instead.")


Even if we add array types, there still gonna be some unsupported types, so warning message can remain.

The warning is handled in the caller

Aha, I see. Maybe we an error would be more in go style.

How about?

// sqlToColumnType maps the type of a SQL column type to a go jet sql builder column. The second return value returns // whether the given type is supported. func sqlToColumnType(typeName string) (string, bool) {

Yeah, that is fine as well.

go-jet · 2024-09-07T11:09:05Z

internal/jet/array_expression.go

+package jet
+
+// ArrayExpression interface
+type ArrayExpression[E Expression] interface {


Maybe just Array[E Expression], so it follows naming convention with Range. Also Expression would repeat twice in the name.

go-jet · 2024-09-07T11:11:04Z

internal/jet/array_expression.go

+	CONCAT(rhs ArrayExpression[E]) ArrayExpression[E]
+	CONCAT_ELEMENT(E) ArrayExpression[E]
+
+	AT(expression IntegerExpression) Expression


AT should return element type E. Take a look at rangeTypeCaster how to extract element type.

go-jet · 2024-09-07T11:15:06Z

internal/jet/expression.go

@@ -316,6 +316,32 @@ func (s *complexExpression) serialize(statement StatementType, out *SQLBuilder,
 	}
 }

+type arraySubscriptExpression struct {


This is the old way to construct custom expression. The new and easier way is using CustomExpression and Token. In this case: CustomExpression(array, Token("["), index, Token("]")).

go-jet · 2024-09-07T11:16:48Z

internal/jet/literal_expression.go

+	literalExpressionImpl
+}
+
+func UnsafeArray[E LiteralExpression](values []interface{}) ArrayExpression[E] {


Why is it needed?

Was experimenting around, forgot to remove it. Gone now

go-jet · 2024-09-07T11:31:22Z

internal/jet/literal_expression.go

+	literalExpressionImpl
+}
+
+func Int64Array(values []int64) ArrayExpression[IntegerExpression] {


Can be done without new literal types. Some of these files need some cleaning.

func Int64Array(values []int64) ArrayExpression[IntegerExpression] { return ArrayExp[IntegerExpression](literal(pq.Int64Array(values)) }

go-jet · 2024-09-07T11:33:23Z

internal/jet/sql_builder.go

@@ -253,6 +255,19 @@ func argToString(value interface{}) string {
 	}
 }

+func stringArrayQuote(val []string) string {


Maybe we can use pq.StringArray{}.Value() instead, and cast first returned value to string.

Like this?

func stringArrayQuote(val []string) string { // We'll rely on the internals of pq2.StringArray here. We know it will never return an error, and the returned // value is a string dv, _ := pq2.StringArray(val).Value() return dv.(string) }

Yeah, exactly.

go-jet · 2024-09-07T11:41:49Z

internal/jet/string_expression.go

@@ -16,6 +16,9 @@ type StringExpression interface {
 	BETWEEN(min, max StringExpression) BoolExpression
 	NOT_BETWEEN(min, max StringExpression) BoolExpression

+	ANY_EQ(rhs ArrayExpression[StringExpression]) BoolExpression
+	ALL_EQ(rhs ArrayExpression[StringExpression]) BoolExpression
+


If we add ANY_EQ, we would also need to add, for each of the types, NOT_EQ_ANY, LT_ANY, LT_EQ_ANY, etc... This would lead to number of methods explosion.

Simpler way would be to have ANY and ALL as generic functions:

func ANY[E Expression](array Array[E]) E { // check rangeTypeCaster }

Then we can write for any type:

Table.SomeStringColumn.EQ(ANY(Table.StringArrayColumn))

Yeah, that makes sense. I'll change that

go-jet · 2024-09-07T11:43:58Z

postgres/cast.go

@@ -45,6 +45,10 @@ type cast interface {
 	AS_INTERVAL() IntervalExpression
 }

+type castArray interface {
+	AS_STRING() jet.ArrayExpression[StringExpression]


Why separate interface?

Was left over cruft from experimentation, removed it

go-jet · 2024-09-07T11:45:30Z

tests/postgres/scan_test.go

@@ -967,26 +969,25 @@ func TestScanIntoCustomBaseTypes(t *testing.T) {
 		ReplacementCost MyFloat64
 		Rating          *model.MpaaRating
 		LastUpdate      MyTime
-		SpecialFeatures *MyString
+		SpecialFeatures MyStringArray


If SpecialFeatures is pq.StringArray you wouldn't need to exclude SpecialFeatures column bellow.

With the type alias like so:

type MyStringArray pq.StringArray

The MyStringArray type does not implement Scanner. The QRM will then see it as a slice because that's what pq.StringArray is and try to apply it's row mapping into the slice. Therefore it will always be null.

Adding a method like this would work:

func (a *MyStringArray) Scan(src interface{}) error { x := (*pq.StringArray)(a) return x.Scan(src) }

What do you think?

No, I'm meant without defining a type, either by realiasing or not using new type at all:

type MyStringArray = pq.StringArray // note '=', MyStringArray now retains Scan method

or

type film struct { FilmID MyUint16 `sql:"primary_key"` Title MyString Description *MyString ReleaseYear *MyInt16 LanguageID MyUint8 RentalDuration MyUint8 RentalRate MyFloat32 Length *MyUint32 ReplacementCost MyFloat64 Rating *model.MpaaRating LastUpdate MyTime SpecialFeatures *MyString SpecialFeatures pq.StringArray }

go-jet · 2024-09-07T11:48:43Z

tests/postgres/alltypes_test.go

@@ -2,6 +2,7 @@ package postgres

 import (
 	"database/sql"
+	"github.com/lib/pq"
 	"testing"
 	"time"



As for testing, maybe we can have a new array only table, similar to what's already been done for range tests.

Note that test files are in separate repo: https://github.com/go-jet/jet-test-data

arjen-ag5 added 12 commits September 3, 2024 15:39

Initial support for postgres text arrays

c38c302

Support arrays in postgres layer

f72150a

Extend generator with text array support

22b845e

Introduced more literal types

a9092e3

Use generic types

f51f32a

Update testcases

e6619f1

Don't use generics in postgres package

566374d

Update generator

d61e0aa

Take array dimensions into consideration

647191a

Use MySQL server container which also support ARM64 to allow running …

b17d5e2

…testcases on macOs

Update test cases for array types

709f631

Inadvertently copied function

1f81d75

arjen-ag5 changed the title ~~Adding support for (string) arrays~~ Adding support for arrays Sep 4, 2024

go-jet reviewed Sep 7, 2024

View reviewed changes

go-jet mentioned this pull request Sep 7, 2024

Please add a PGX adapter #381

Open

Add a function to map SQL array types

220034e

arjen-ag5 added 6 commits September 7, 2024 20:01

Renamed ArrayExpression to Array

de67d7d

AT now returns it's elements generic type

cd7160a

Use CustomExpression

5d0cd24

Removed old cruft

e4f6a51

Cleanup literal type construction

261df9d

Use PQ creating a driver value for arrays

1d19220

Adding support for arrays #380

Are you sure you want to change the base?

Adding support for arrays #380

Conversation

arjen-ag5 commented Sep 4, 2024

go-jet commented Sep 7, 2024

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

arjen-ag5 Sep 9, 2024 • edited Loading

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

arjen-ag5 Sep 7, 2024 • edited Loading

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

go-jet Sep 7, 2024 • edited Loading

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

arjen-ag5 Sep 9, 2024 • edited Loading

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

arjen-ag5 Sep 9, 2024 •

edited

Loading

arjen-ag5 Sep 7, 2024 •

edited

Loading

go-jet Sep 7, 2024 •

edited

Loading

arjen-ag5 Sep 9, 2024 •

edited

Loading