Skip to content

Conversation

@disq
Copy link
Member

@disq disq commented Jul 4, 2023

Refer to the comment here for explanation: cloudquery/cloudquery#11724 (comment)

diff from v3.10.6:

diff --git a/schema/testdata.go b/schema/testdata.go
index 5570c6a..bd8b54c 100644
--- a/schema/testdata.go
+++ b/schema/testdata.go
@@ -12,7 +12,7 @@ import (
 	"github.com/apache/arrow/go/v13/arrow"
 	"github.com/apache/arrow/go/v13/arrow/array"
 	"github.com/apache/arrow/go/v13/arrow/memory"
-	"github.com/cloudquery/plugin-sdk/v3/types"
+	"github.com/cloudquery/plugin-sdk/v4/types"
 	"github.com/google/uuid"
 	"golang.org/x/exp/rand"
 	"golang.org/x/exp/slices"
@@ -21,7 +21,6 @@ import (
 // TestSourceOptions controls which types are included by TestSourceColumns.
 type TestSourceOptions struct {
 	SkipDates      bool
-	SkipDecimals   bool
 	SkipDurations  bool
 	SkipIntervals  bool
 	SkipLargeTypes bool // e.g. large binary, large string
@@ -31,16 +30,12 @@ type TestSourceOptions struct {
 	SkipTimes      bool // time of day types
 	SkipTimestamps bool // timestamp types. Microsecond timestamp is always be included, regardless of this setting.
 	TimePrecision  time.Duration
+	SkipDecimals   bool
 }
 
 // TestSourceColumns returns columns for all Arrow types and composites thereof. TestSourceOptions controls
 // which types are included.
 func TestSourceColumns(testOpts TestSourceOptions) []Column {
-	// cq columns
-	var cqColumns []Column
-	cqColumns = append(cqColumns, Column{Name: CqIDColumn.Name, Type: types.NewUUIDType(), NotNull: true, Unique: true, PrimaryKey: true})
-	cqColumns = append(cqColumns, Column{Name: CqParentIDColumn.Name, Type: types.NewUUIDType()})
-
 	var basicColumns []Column
 	basicColumns = append(basicColumns, primitiveColumns()...)
 	basicColumns = append(basicColumns, binaryColumns()...)
@@ -116,11 +111,10 @@ func TestSourceColumns(testOpts TestSourceOptions) []Column {
 		compositeColumns = append(compositeColumns, Column{Name: "struct", Type: arrow.StructOf(columnsToFields(basicColumns...)...)})
 
 		// struct with nested struct
-		compositeColumns = append(compositeColumns, Column{Name: "nested_struct", Type: arrow.StructOf(arrow.Field{Name: "inner", Type: arrow.StructOf(columnsToFields(basicColumns...)...)})})
+		// compositeColumns = append(compositeColumns, Column{Name: "nested_struct", Type: arrow.StructOf(arrow.Field{Name: "inner", Type: arrow.StructOf(columnsToFields(basicColumns...)...)})})
 	}
 
-	allColumns := append(append(cqColumns, basicColumns...), compositeColumns...)
-	return allColumns
+	return append(basicColumns, compositeColumns...)
 }
 
 // primitiveColumns returns a list of primitive columns as defined by Arrow types.
@@ -246,13 +240,7 @@ func columnsToFields(columns ...Column) []arrow.Field {
 
 // TestTable returns a table with columns of all types. Useful for destination testing purposes
 func TestTable(name string, testOpts TestSourceOptions) *Table {
-	var columns []Column
-	// columns = append(columns, Column{Name: "uuid", Type: types.NewUUIDType()})
-	// columns = append(columns, Column{Name: "string_pk", Type: arrow.BinaryTypes.String})
-	columns = append(columns, Column{Name: CqSourceNameColumn.Name, Type: arrow.BinaryTypes.String})
-	columns = append(columns, Column{Name: CqSyncTimeColumn.Name, Type: arrow.FixedWidthTypes.Timestamp_us})
-	columns = append(columns, TestSourceColumns(testOpts)...)
-	return &Table{Name: name, Columns: columns}
+	return &Table{Name: name, Columns: TestSourceColumns(testOpts)}
 }
 
 // GenTestDataOptions are options for generating test data

@disq disq requested a review from yevgenypats as a code owner July 4, 2023 09:46
@github-actions github-actions bot added fix and removed fix labels Jul 4, 2023
@hermanschaaf
Copy link
Member

What is the actual difference in terms of type coverage? Can't we just add the missing ones (if any?)

@disq
Copy link
Member Author

disq commented Jul 4, 2023

What is the actual difference in terms of type coverage? Can't we just add the missing ones (if any?)

If we add anything manually it would have to be maintained if new types show up in Arrow.

Some types (some intervals and some "simples" like boolean) are missing, you can work it out from the diff here.

@disq disq requested a review from candiduslynx July 4, 2023 11:18
@hermanschaaf
Copy link
Member

If we add anything manually it would have to be maintained if new types show up in Arrow.

I think that's okay (saying this as the person who originally wrote the code looping over Arrow types); we don't have to support new Arrow types immediately after their release, and we get simpler code if we don't loop over the types.

I would vote to add the couple of missing types for now.

@disq disq closed this Jul 4, 2023
@disq disq deleted the fix/bring-back-all-test-types branch July 4, 2023 11:38
kodiakhq bot pushed a commit that referenced this pull request Jul 4, 2023
Just add the missing ones. Supercedes #1057
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