feat(duckdb): Update to SDK V2 (Arrow) #10275

yevgenypats · 2023-04-24T12:12:31Z

BEGIN_COMMIT_OVERRIDE
feat: Update to use Apache Arrow type system

BREAKING-CHANGE: This release introduces an internal change to our type system to use Apache Arrow. This should not have any visible breaking changes, however due to the size of the change we are introducing it under a major version bump to communicate that it might have some bugs that we weren't able to catch during our internal tests. If you encounter an issue during the upgrade, please submit a bug report.

END_COMMIT_OVERRIDE

plugins/destination/duckdb/json/writer.go

hermanschaaf · 2023-04-25T08:20:59Z

plugins/destination/duckdb/client/types.go

+	case "int", "int4", "integer":
+		return arrow.PrimitiveTypes.Uint32


This doesn't look right; it probably shouldn't be unsigned?

(also the ones above it)

hermanschaaf · 2023-04-25T08:24:37Z

plugins/destination/duckdb/client/types.go

+	case "blob", "bytea", "binary", "varbinary":
+		return arrow.BinaryTypes.Binary
+	case "date":
+		return arrow.FixedWidthTypes.Date64


Suggested change

return arrow.FixedWidthTypes.Date64

return arrow.FixedWidthTypes.Date32

We need to be careful with Date64 - it stores the date in millisecond precision, for reasons that I don't understand. I think what we need to use here is Date32, which stores the number of days since unix epoch.

That's weird. as far as I understand this is just the width of the array....I looked again but I don't see that in arrow...can you point me to that?

This is what I'm basing it on: https://arrow.apache.org/docs/python/generated/pyarrow.date64.html

And e.g. here, when converting to time.Time, it throws away all the millisecond precision being stored anyway and just returns the date (year/month/day) https://github.com/apache/arrow/blob/72191880d3a66e6ecf65d9c249dcb2856a697f47/go/arrow/datatype_fixedwidth.go#L99-L102

seems it's a python thing but yeah it should matter anyway. ok will change it if you were bitten by that already

Yeah, I had a hard-to-debug error at some point where two dates like 2021-01-01 and 2021-01-01 were coming back as different, and it turned out to be a difference in milliseconds that isn't printed by the date64 string output.

hermanschaaf · 2023-04-25T08:27:21Z

plugins/destination/duckdb/client/write.go

-			if err != nil {
-				return fmt.Errorf("failed to execute upsert: %w", err)
-			}
+// func (c *Client) upsert(table *schema.Table, data []any) error {


This commented out section can probably be removed now

hermanschaaf · 2023-04-25T09:09:05Z

plugins/destination/duckdb/json/writer.go

+					} else {
+						ind := strings.Index(strTimestamp, ".")
+						if len(strTimestamp[ind+1:]) < 6 {
+							tmp[w.schema.Field(j).Name] = strTimestamp + "000000"[len(strTimestamp[ind+1:]):]


Wow, I actually didn't know you can do "000000"[i:i+1] in Go without assigning a variable first 😄

An alternative would be to use strings.Repeat("0", 6 - len(strTimestamp[ind+1:])

plugins/destination/duckdb/client/write.go

plugins/destination/duckdb/main.go

hermanschaaf

We need to add destination.WithManagedWriter()

erezrokah

Added a few comments (non blocking) and will run locally too

erezrokah · 2023-04-25T10:54:33Z

plugins/destination/duckdb/client/read.go

-		}
+func (c *Client) Read(ctx context.Context, sc *arrow.Schema, sourceName string, res chan<- arrow.Record) error {
+	tableName := schema.TableName(sc)
+	f, err := os.CreateTemp("", fmt.Sprintf("%s-*.json", tableName))


Should we remove this temporary file when we're done with it?

erezrokah · 2023-04-25T10:57:51Z

plugins/destination/duckdb/client/types.go

-		return "text[]"
-	case schema.TypeIntArray:
-		return "int[]"
+	case *arrow.Date64Type:


Suggested change

case *arrow.Date64Type:

case *arrow.Date32Type:

If we do https://github.com/cloudquery/cloudquery/pull/10275/files#r1176178054 cc @hermanschaaf

I wasn't able to find that it stores it in milliseconds in the code

What do you mean? That's how the type is defined here https://arrow.apache.org/docs/python/generated/pyarrow.date64.html#pyarrow.date64 "milliseconds since UNIX epoch 1970-01-01"?

I think it's a python thing but anyway changed it as prob there is no reason for date64 anyway as date32 should be enough

erezrokah · 2023-04-25T10:59:29Z

plugins/destination/duckdb/client/types.go

+		return arrow.PrimitiveTypes.Uint8
+	case "smallint":
+		return arrow.PrimitiveTypes.Uint16
+	case "int", "int4", "integer":


Do we need int, int4? Those are aliases for integer. And if we do need them, should we also added signed to this case, and all aliases to other cases as specified in https://duckdb.org/docs/sql/data_types/overview.html?

For example tinyint has an int1 alias

Added the alias. Yeah it's better if handle the aliases as well (no reason not to handle them I think)

Co-authored-by: Kemal <[email protected]>

Missed from my CR at #10275

feat(duckdb): Update to SDK V2 (Arrow)

2ee59d9

cq-bot added the duckdb label Apr 24, 2023

disq linked an issue Apr 24, 2023 that may be closed by this pull request

feat: Migrate plugin/destination/duckdb to github.com/cloudquery/plugin-sdk/v2 #10103

Closed

1 task

yevgenypats added 4 commits April 24, 2023 19:20

wip

4f23cc3

tests passing

8bee22e

fix duckdb

f75a67a

fix

875b040

yevgenypats marked this pull request as ready for review April 24, 2023 20:40

yevgenypats requested review from a team and candiduslynx and removed request for a team April 24, 2023 20:41

Merge branch 'main' into feat/duckdb_arrow

25a42ae

yevgenypats requested review from a team and hermanschaaf and removed request for candiduslynx April 24, 2023 20:41

vercel bot deployed to Preview April 24, 2023 20:45 View deployment

disq reviewed Apr 25, 2023

View reviewed changes

plugins/destination/duckdb/json/writer.go Outdated Show resolved Hide resolved

hermanschaaf reviewed Apr 25, 2023

View reviewed changes

plugins/destination/duckdb/client/write.go Show resolved Hide resolved

hermanschaaf reviewed Apr 25, 2023

View reviewed changes

plugins/destination/duckdb/main.go Show resolved Hide resolved

hermanschaaf requested changes Apr 25, 2023

View reviewed changes

Use managed writer

4819d65

erezrokah reviewed Apr 25, 2023

View reviewed changes

yevgenypats and others added 4 commits April 25, 2023 17:51

Update plugins/destination/duckdb/json/writer.go

7ca3f14

Co-authored-by: Kemal <[email protected]>

fix review

e864baf

fix review

980beb7

fix review

52ced15

yevgenypats requested review from disq, erezrokah and hermanschaaf April 25, 2023 15:22

fix review

74465ee

hermanschaaf approved these changes Apr 25, 2023

View reviewed changes

yevgenypats added the automerge Automatically merge once required checks pass label Apr 25, 2023

Merge branch 'main' into feat/duckdb_arrow

0f6c59e

vercel bot deployed to Preview April 25, 2023 20:27 View deployment

kodiakhq bot merged commit e38eae6 into main Apr 25, 2023

kodiakhq bot deleted the feat/duckdb_arrow branch April 25, 2023 20:28

disq mentioned this pull request Apr 26, 2023

chore(duckdb): Simplify JSON writer #10364

Merged

kodiakhq bot pushed a commit that referenced this pull request Apr 26, 2023

chore(duckdb): Simplify JSON writer (#10364)

0d82c19

Missed from my CR at #10275

		case "int", "int4", "integer":
		return arrow.PrimitiveTypes.Uint32

	return arrow.FixedWidthTypes.Date64
	return arrow.FixedWidthTypes.Date32

feat(duckdb): Update to SDK V2 (Arrow) #10275

feat(duckdb): Update to SDK V2 (Arrow) #10275

Uh oh!

Conversation

yevgenypats commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yevgenypats commented Apr 24, 2023 •

edited

Loading

erezrokah Apr 25, 2023 •

edited

Loading