This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new e80b21f refactor(c/driver/common): Variadic arguments for
StringBuilderAppend (#587)
e80b21f is described below
commit e80b21fab2ce8078c43a8e4d5ae73ea448e640ee
Author: William Ayd <[email protected]>
AuthorDate: Thu Apr 27 19:37:01 2023 -0700
refactor(c/driver/common): Variadic arguments for StringBuilderAppend (#587)
Cuts down on the verbosity of string appends and bridges the functional
gap between this and the templated C++ version a bit.
In follow ups would be nice to support non-string arguments, clear up
the error handling, and make the Append function non-void with a return
value
---
c/driver/common/CMakeLists.txt | 15 ++++++++
c/driver/common/utils.c | 58 +++++++++++++++++++----------
c/driver/common/utils.h | 19 +++++++++-
c/driver/common/utils_test.cc | 74 +++++++++++++++++++++++++++++++++++++
c/driver/sqlite/sqlite.c | 83 +++++++++++++++++++++++++++++++++---------
5 files changed, 210 insertions(+), 39 deletions(-)
diff --git a/c/driver/common/CMakeLists.txt b/c/driver/common/CMakeLists.txt
index 6a9cd54..5eaa307 100644
--- a/c/driver/common/CMakeLists.txt
+++ b/c/driver/common/CMakeLists.txt
@@ -19,3 +19,18 @@ add_library(adbc_driver_common STATIC utils.c)
set_target_properties(adbc_driver_common PROPERTIES POSITION_INDEPENDENT_CODE
ON)
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
+
+if(ADBC_BUILD_TESTS)
+ add_test_case(driver_common_test
+ PREFIX
+ adbc
+ SOURCES
+ utils_test.cc
+ EXTRA_LINK_LIBS
+ adbc_driver_common
+ nanoarrow)
+ target_compile_features(adbc-driver-common-test PRIVATE cxx_std_17)
+ adbc_configure_target(adbc-driver-common-test)
+endif()
+
+validate_config()
diff --git a/c/driver/common/utils.c b/c/driver/common/utils.c
index 1e6d814..8ea35c5 100644
--- a/c/driver/common/utils.c
+++ b/c/driver/common/utils.c
@@ -23,12 +23,13 @@
#include <stdlib.h>
#include <string.h>
+#include <assert.h>
#include <nanoarrow/nanoarrow.h>
static size_t kErrorBufferSize = 256;
static char kErrorPrefix[] = "[SQLite] ";
-void ReleaseError(struct AdbcError* error) {
+static void ReleaseError(struct AdbcError* error) {
free(error->message);
error->message = NULL;
error->release = NULL;
@@ -58,11 +59,11 @@ struct SingleBatchArrayStream {
struct ArrowSchema schema;
struct ArrowArray batch;
};
-const char* SingleBatchArrayStreamGetLastError(struct ArrowArrayStream*
stream) {
+static const char* SingleBatchArrayStreamGetLastError(struct ArrowArrayStream*
stream) {
return NULL;
}
-int SingleBatchArrayStreamGetNext(struct ArrowArrayStream* stream,
- struct ArrowArray* batch) {
+static int SingleBatchArrayStreamGetNext(struct ArrowArrayStream* stream,
+ struct ArrowArray* batch) {
if (!stream || !stream->private_data) return EINVAL;
struct SingleBatchArrayStream* impl =
(struct SingleBatchArrayStream*)stream->private_data;
@@ -71,15 +72,15 @@ int SingleBatchArrayStreamGetNext(struct ArrowArrayStream*
stream,
memset(&impl->batch, 0, sizeof(*batch));
return 0;
}
-int SingleBatchArrayStreamGetSchema(struct ArrowArrayStream* stream,
- struct ArrowSchema* schema) {
+static int SingleBatchArrayStreamGetSchema(struct ArrowArrayStream* stream,
+ struct ArrowSchema* schema) {
if (!stream || !stream->private_data) return EINVAL;
struct SingleBatchArrayStream* impl =
(struct SingleBatchArrayStream*)stream->private_data;
return ArrowSchemaDeepCopy(&impl->schema, schema);
}
-void SingleBatchArrayStreamRelease(struct ArrowArrayStream* stream) {
+static void SingleBatchArrayStreamRelease(struct ArrowArrayStream* stream) {
if (!stream || !stream->private_data) return;
struct SingleBatchArrayStream* impl =
(struct SingleBatchArrayStream*)stream->private_data;
@@ -119,24 +120,41 @@ AdbcStatusCode BatchToArrayStream(struct ArrowArray*
values, struct ArrowSchema*
return ADBC_STATUS_OK;
}
-void StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
+int StringBuilderInit(struct StringBuilder* builder, size_t initial_size) {
builder->buffer = (char*)malloc(initial_size);
+ if (builder->buffer == NULL) return errno;
+
builder->size = 0;
builder->capacity = initial_size;
+
+ return 0;
}
-void StringBuilderAppend(struct StringBuilder* builder, const char* value) {
- size_t length = strlen(value);
- size_t new_size = builder->size + length;
- if (new_size > builder->capacity) {
- size_t new_capacity = builder->size + length - builder->capacity;
- if (builder->size == 0) new_capacity++;
-
- builder->buffer = realloc(builder->buffer, new_capacity);
- builder->capacity = new_capacity;
+int StringBuilderAppend(struct StringBuilder* builder, const char* fmt, ...) {
+ va_list argptr;
+ int bytes_available = builder->capacity - builder->size;
+
+ va_start(argptr, fmt);
+ int n = vsnprintf(builder->buffer + builder->size, bytes_available, fmt,
argptr);
+ va_end(argptr);
+
+ if (n < 0) {
+ return errno;
+ } else if (n >= bytes_available) { // output was truncated
+ int bytes_needed = n - bytes_available + 1;
+ builder->buffer = (char*)realloc(builder->buffer, builder->capacity +
bytes_needed);
+ if (builder->buffer == NULL) return errno;
+
+ builder->capacity += bytes_needed;
+
+ va_start(argptr, fmt);
+ int ret = vsnprintf(builder->buffer + builder->size, n + 1, fmt, argptr);
+ assert(ret >= 0);
+
+ va_end(argptr);
}
- memcpy(builder->buffer + builder->size, value, length);
- builder->buffer[new_size] = '\0';
- builder->size = new_size;
+ builder->size += n;
+
+ return 0;
}
void StringBuilderReset(struct StringBuilder* builder) {
if (builder->buffer) {
diff --git a/c/driver/common/utils.h b/c/driver/common/utils.h
index 3fc5bc5..0223dca 100644
--- a/c/driver/common/utils.h
+++ b/c/driver/common/utils.h
@@ -27,6 +27,10 @@
#define SET_ERROR_ATTRIBUTE
#endif
+#ifdef __cplusplus
+extern "C" {
+#endif
+
/// Set error details using a format string.
void SetError(struct AdbcError* error, const char* format, ...)
SET_ERROR_ATTRIBUTE;
@@ -43,8 +47,15 @@ struct StringBuilder {
size_t size;
size_t capacity;
};
-void StringBuilderInit(struct StringBuilder* builder, size_t initial_size);
-void StringBuilderAppend(struct StringBuilder* builder, const char* value);
+int StringBuilderInit(struct StringBuilder* builder, size_t initial_size);
+
+#if defined(__GNUC__)
+#define ADBC_STRING_BUILDER_FORMAT_CHECK __attribute__((format(printf, 2, 3)))
+#else
+#define ADBC_STRING_BUILDER_FORMAT_CHECK
+#endif
+int ADBC_STRING_BUILDER_FORMAT_CHECK StringBuilderAppend(struct StringBuilder*
builder,
+ const char* fmt, ...);
void StringBuilderReset(struct StringBuilder* builder);
/// Check an NanoArrow status code.
@@ -92,3 +103,7 @@ void StringBuilderReset(struct StringBuilder* builder);
AdbcStatusCode adbc_status_code = (EXPR); \
if (adbc_status_code != ADBC_STATUS_OK) return adbc_status_code; \
} while (0)
+
+#ifdef __cplusplus
+}
+#endif
diff --git a/c/driver/common/utils_test.cc b/c/driver/common/utils_test.cc
new file mode 100644
index 0000000..6fa7e25
--- /dev/null
+++ b/c/driver/common/utils_test.cc
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "utils.h"
+
+TEST(TestStringBuilder, TestBasic) {
+ struct StringBuilder str;
+ int ret;
+ ret = StringBuilderInit(&str, /*initial_size=*/64);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.capacity, 64);
+
+ ret = StringBuilderAppend(&str, "%s", "BASIC TEST");
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.size, 10);
+ EXPECT_STREQ(str.buffer, "BASIC TEST");
+
+ StringBuilderReset(&str);
+}
+
+TEST(TestStringBuilder, TestBoundary) {
+ struct StringBuilder str;
+ int ret;
+ ret = StringBuilderInit(&str, /*initial_size=*/10);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.capacity, 10);
+
+ ret = StringBuilderAppend(&str, "%s", "BASIC TEST");
+ EXPECT_EQ(ret, 0);
+ // should resize to include \0
+ EXPECT_EQ(str.capacity, 11);
+ EXPECT_EQ(str.size, 10);
+ EXPECT_STREQ(str.buffer, "BASIC TEST");
+
+ StringBuilderReset(&str);
+}
+
+TEST(TestStringBuilder, TestMultipleAppends) {
+ struct StringBuilder str;
+ int ret;
+ ret = StringBuilderInit(&str, /*initial_size=*/2);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.capacity, 2);
+
+ ret = StringBuilderAppend(&str, "%s", "BASIC");
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.capacity, 6);
+ EXPECT_EQ(str.size, 5);
+ EXPECT_STREQ(str.buffer, "BASIC");
+
+ ret = StringBuilderAppend(&str, "%s", " TEST");
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(str.capacity, 11);
+ EXPECT_EQ(str.size, 10);
+ EXPECT_STREQ(str.buffer, "BASIC TEST");
+
+ StringBuilderReset(&str);
+}
diff --git a/c/driver/sqlite/sqlite.c b/c/driver/sqlite/sqlite.c
index dab5e72..2cc3276 100644
--- a/c/driver/sqlite/sqlite.c
+++ b/c/driver/sqlite/sqlite.c
@@ -1022,9 +1022,16 @@ AdbcStatusCode SqliteConnectionGetTableSchema(struct
AdbcConnection* connection,
}
struct StringBuilder query = {0};
- StringBuilderInit(&query, /*initial_size=*/64);
- StringBuilderAppend(&query, "SELECT * FROM ");
- StringBuilderAppend(&query, table_name);
+ if (StringBuilderInit(&query, /*initial_size=*/64) != 0) {
+ SetError(error, "Could not initiate StringBuilder");
+ return ADBC_STATUS_INTERNAL;
+ }
+
+ if (StringBuilderAppend(&query, "%s%s", "SELECT * FROM ", table_name) != 0) {
+ StringBuilderReset(&query);
+ SetError(error, "Call to StringBuilderAppend failed");
+ return ADBC_STATUS_INTERNAL;
+ }
sqlite3_stmt* stmt = NULL;
int rc =
@@ -1222,27 +1229,67 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
// Create statements for CREATE TABLE / INSERT
struct StringBuilder create_query = {0};
struct StringBuilder insert_query = {0};
- StringBuilderInit(&create_query, 256);
- StringBuilderInit(&insert_query, 256);
- StringBuilderAppend(&create_query, "CREATE TABLE ");
- StringBuilderAppend(&create_query, stmt->target_table);
- StringBuilderAppend(&create_query, " (");
+ if (StringBuilderInit(&create_query, /*initial_size=*/256) != 0) {
+ SetError(error, "Could not initiate StringBuilder");
+ return ADBC_STATUS_INTERNAL;
+ }
+
+ if (StringBuilderInit(&insert_query, /*initial_size=*/256) != 0) {
+ SetError(error, "Could not initiate StringBuilder");
+ StringBuilderReset(&create_query);
+ return ADBC_STATUS_INTERNAL;
+ }
+
+ if (StringBuilderAppend(&create_query, "%s%s%s", "CREATE TABLE ",
stmt->target_table,
+ " (") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
- StringBuilderAppend(&insert_query, "INSERT INTO ");
- StringBuilderAppend(&insert_query, stmt->target_table);
- StringBuilderAppend(&insert_query, " VALUES (");
+ if (StringBuilderAppend(&insert_query, "%s%s%s", "INSERT INTO ",
stmt->target_table,
+ " VALUES (") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
for (int i = 0; i < stmt->binder.schema.n_children; i++) {
- if (i > 0) StringBuilderAppend(&create_query, ", ");
+ if (i > 0) StringBuilderAppend(&create_query, "%s", ", ");
// XXX: should escape the column name too
- StringBuilderAppend(&create_query, stmt->binder.schema.children[i]->name);
+ if (StringBuilderAppend(&create_query, "%s",
stmt->binder.schema.children[i]->name) !=
+ 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
+
+ if (i > 0) {
+ if (StringBuilderAppend(&insert_query, "%s", ", ") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
+ }
- if (i > 0) StringBuilderAppend(&insert_query, ", ");
- StringBuilderAppend(&insert_query, "?");
+ if (StringBuilderAppend(&insert_query, "%s", "?") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
+ }
+ if (StringBuilderAppend(&create_query, "%s", ")") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
+
+ if (StringBuilderAppend(&insert_query, "%s", ")") != 0) {
+ SetError(error, "Call to StringBuilderAppend failed");
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
}
- StringBuilderAppend(&create_query, ")");
- StringBuilderAppend(&insert_query, ")");
sqlite3_stmt* create = NULL;
if (!stmt->append) {
@@ -1271,6 +1318,8 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
}
sqlite3_finalize(create);
+
+cleanup:
StringBuilderReset(&create_query);
StringBuilderReset(&insert_query);
return code;
