Add support for DuckDB Appender API #37

panta · 2022-08-02T09:04:48Z

Besides the support for the aforementioned Appender API, this also

adds explicit support for various numerical types (UTINYINT, USMALLINT, ...) in particular to add column type support
add more fuzzing tests for the various basic types
switch to use duckdb_open_ext() in place of duckdb_open() to be able to return error information

marcboeker · 2022-08-03T21:16:14Z

@panta Thank you for this PR. Please give me some days to review this, as I'm currently quite busy.

panta · 2022-08-04T08:09:57Z

@panta Thank you for this PR. Please give me some days to review this, as I'm currently quite busy.

Sure, thanks. Just a thing I forgot to mention: tests for the appender use filesystem backed DBs (using temporary files), because the appender API doesn't seem to work with memory DBs.

marcboeker · 2022-08-03T21:24:44Z

appender.go

+
+		switch v := v.(type) {
+		case uint8:
+			if rv := C.duckdb_append_uint8(*a.appender, C.uint8_t(v)); rv == C.DuckDBError {


Would it be possible to define rv outside of the switch statement and just do a

rv := C.duckdb_append_uint8(*a.appender, C.uint8_t(v))

and after the switch statement check rv for an error and in case return the error? This would probably reduce some duplicate code except for string and time.Time.

Hey, thanks for the feedback, and I apologize for the delay. In the next few days I'll resume the work on this and address the issues with the PR. Thanks!

I've pushed an additional commit that addresses all the points.
If you deem it ok, I could squash the appender commits into one before merging, so we have a cleaner history.
Please let me know.

Thanks for the additional commits. There is currently one big PR ongoing, which should be merged shortly. I would suggest to wait for the merge and check, if your modifications still work. Would that be OK for you?

@panta The PR has been merged. Could you please check if your branch still works with the modifications. Thanks 🙂

marcboeker · 2022-08-03T21:30:54Z

appender_test.go

+)
+
+const (
+	testAppenderTableDdl = `CREATE TABLE appdata(id BIGINT, u8 UTINYINT, i8 TINYINT, u16 USMALLINT, i16 SMALLINT,


I would suggest to call it DDL instead of Ddl as per the Go naming convention for acronyms.

marcboeker · 2022-08-05T15:15:59Z

appender.go

+	"unsafe"
+)
+
+type appender struct {


Wouldn't it make sense to make the appender exported? Then it would also appear in the documentation.

marcboeker · 2022-08-05T15:19:28Z

appender.go

+	// if state := C.duckdb_appender_flush(*a.appender); state == C.DuckDBError {
+	// 	dbErr := C.GoString(C.duckdb_appender_error(*a.appender))
+	// 	return errors.New(dbErr)
+	// }


Can we remove this part as duckdb_appender_destroy should be sufficient?

marcboeker · 2022-08-05T15:27:19Z

connection.go

+	return &appender{c: c, schema: schema, table: table, appender: &a}, nil
+}
+
+func Appender(dbconn driver.Conn, schema string, table string) (*appender, error) {


I would suggest to add a NewAppenderFromConn(conn driver.Conn, schema, table string) (*Appender, error) method to the appender.go and move the code from (c *conn) Appender() there. This would keep the current DuckDB implementation close to the original database/sql design idea and all extensions, like the appender, would be more separated. What do you think about this?

marcboeker · 2022-08-05T15:31:24Z

appender.go

+				return fmt.Errorf("couldn't append parameter %d (type %T)", i, v)
+			}
+		case []byte:
+			if v == nil {


Isn't this already checked for in line 58?

marcboeker · 2022-08-05T15:32:18Z

appender_test.go

+
+		appender, err := Appender(llConn, "", "appdata")
+		require.NoError(t, err)
+		// defer appender.Close()


Wouldn't this be more safe than a manual appender.Close() in line 183?

marcboeker · 2022-08-05T15:32:35Z

appender_test.go

+	return &res
+}
+
+func TestAppenderSimple(t *testing.T) {


Maybe we could only use TestRandomizedAppender instead of TestAppenderSimple as it already covers everything that the simple test case does.

…, and large number of randomized rows).

…NT, (U)INTEGER, (U)BIGINT, REAL, DOUBLE.

…aseTypeName()).

… return meaningful error messages in case of error.

* Reduce code duplication in appender.AppendRowArray() * Rename testAppenderTableDdl to testAppenderTableDDL. * Rename Appender() to NewAppenderFromConn(). * Move NewAppenderFromConn() to appender.go and remove conn.Appender(). * Remove commented unnecessary code from appender.Close() * Export the appender type (rename it from appender to Appender). * Add documentation comments to Appender and its methods. * FIX: typecast the driver connection interface to the proper interface before passing to NewAppenderFromConn(). * Remove reduntant nil handling in AppendRowArray(). * Remove unnecessary TestAppenderSimple (the same test cases are covered by the more thorough TestRandomizedAppender). * Remove commented defer and add an explanation for the explicit Close() call.

…lumnTypes().

marcboeker · 2022-10-19T19:40:32Z

Thanks for all the updates and sorry for my late response. As the PR got quite big I would suggest to extract the testing part to a separate PR and start with a smaller PR just for the appender API. I've seen, that there are merge conflicts. Maybe you could merge the current master beforehand.

Regarding the tests: I don't quite see a huge difference compared to the current tests. What do you think about using real fuzzing provided by Go 1.18?

flarco · 2023-02-09T16:13:14Z

Perhaps I can help get this to a merging state. @marcboeker you stated to split the tests and api changes into two different PRs. I can do that. Anything else you think needs to be done?

marcboeker · 2023-02-09T16:20:47Z

@flarco Sounds great. I think it would make sense to merge the current master into this branch, as there are could be some breaking changes.

The idea behind splitting the tests was, that some tests are not related to the appender feature. For now I would suggest to exclude non related tests and only keep the appender relates ones in the PR.

flarco · 2023-02-10T23:16:26Z

@marcboeker opened #71 and removed the non-appender related tests. master was merged into it and there are no conflicts at the moment. Please review and approve.

panta changed the title ~~Add support for DuckDB Appender APIcccccckliludifnfbjgfrnjflunlrbkfflilvbbbjkdh~~ Add support for DuckDB Appender API Aug 2, 2022

marcboeker reviewed Aug 5, 2022

View reviewed changes

panta force-pushed the panta/feature/appender-support branch 2 times, most recently from 711c21f to ef45a79 Compare October 14, 2022 09:37

panta added 8 commits October 17, 2022 09:35

Add more extensive tests for VARCHARs (all lenghts between 0 and 1024…

89584b0

…, and large number of randomized rows).

Add randomized tests for basic numerical types: (U)TINYINT, (U)SMALLI…

9a1507b

…NT, (U)INTEGER, (U)BIGINT, REAL, DOUBLE.

Add test case for column type names (returned by rows.ColumnTypeDatab…

f0fe5a3

…aseTypeName()).

Always use C.duckdb_open_ext() instead of C.duckdb_open() in order to…

05ded6c

… return meaningful error messages in case of error.

Add DuckDB appender support.

39c0ff5

Avoid microsend rouding related test errors on linux.

ab4a9f4

Add unit test for TIMESTAMP column type - tests INSERT, SELECT and Co…

e7bf1e1

…lumnTypes().

panta force-pushed the panta/feature/appender-support branch from ef45a79 to e7bf1e1 Compare October 17, 2022 07:39

marcboeker mentioned this pull request Feb 9, 2023

Implement Appender #70

Closed

flarco mentioned this pull request Feb 10, 2023

Add support for DuckDB Appender API [New] #71

Merged

marcboeker merged commit 9916032 into marcboeker:master Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for DuckDB Appender API #37

Add support for DuckDB Appender API #37

panta commented Aug 2, 2022 •

edited

Loading

marcboeker commented Aug 3, 2022

panta commented Aug 4, 2022

marcboeker Aug 3, 2022

panta Sep 26, 2022

panta Sep 27, 2022

marcboeker Sep 27, 2022

marcboeker Sep 27, 2022

marcboeker Aug 3, 2022

marcboeker Aug 5, 2022

marcboeker Aug 5, 2022

marcboeker Aug 5, 2022

marcboeker Aug 5, 2022

marcboeker Aug 5, 2022

marcboeker Aug 5, 2022

marcboeker commented Oct 19, 2022

flarco commented Feb 9, 2023

marcboeker commented Feb 9, 2023

flarco commented Feb 10, 2023

Add support for DuckDB Appender API #37

Add support for DuckDB Appender API #37

Conversation

panta commented Aug 2, 2022 • edited Loading

marcboeker commented Aug 3, 2022

panta commented Aug 4, 2022

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

marcboeker commented Oct 19, 2022

flarco commented Feb 9, 2023

marcboeker commented Feb 9, 2023

flarco commented Feb 10, 2023

panta commented Aug 2, 2022 •

edited

Loading