[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #900: test(c): Timestamp testing improvements

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #900:
URL: https://github.com/apache/arrow-adbc/pull/900#discussion_r1261540990


##########
c/driver/sqlite/sqlite_test.cc:
##########
@@ -39,8 +39,7 @@ class SqliteQuirks : public adbc_validation::DriverQuirks {
   AdbcStatusCode SetupDatabase(struct AdbcDatabase* database,
                                struct AdbcError* error) const override {
     // Shared DB required for transaction tests
-    return AdbcDatabaseSetOption(
-        database, "uri", "file:Sqlite_Transactions?mode=memory&cache=shared", error);
+    return AdbcDatabaseSetOption(database, "uri", "file:Sqlite_Transactions", error);

Review Comment:
   Hmm, why was this changed? This is specifically to make sure two connections in the same process can share the same underlying database



##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -576,6 +576,91 @@ class PostgresStatementTest : public ::testing::Test,
   }
 
  protected:
+  void TestSqlIngestTemporalType(enum ArrowTimeUnit unit, const char* timezone) override {
+    if (!quirks()->supports_bulk_ingest()) {
+      GTEST_SKIP();
+    }
+
+    ASSERT_THAT(quirks()->DropTable(&connection, "bulk_ingest", &error),
+                adbc_validation::IsOkStatus(&error));
+
+    adbc_validation::Handle<struct ArrowSchema> schema;
+    adbc_validation::Handle<struct ArrowArray> array;
+    struct ArrowError na_error;
+    const std::vector<std::optional<int64_t>> values = {std::nullopt, -42, 0, 42};
+    const ArrowType type = NANOARROW_TYPE_TIMESTAMP;
+
+    // much of this code is shared with TestSqlIngestType with minor
+    // changes to allow for various time units to be tested
+    ArrowSchemaInit(&schema.value);
+    ArrowSchemaSetTypeStruct(&schema.value, 1);
+    ArrowSchemaSetTypeDateTime(schema->children[0], type, unit, timezone);
+    ArrowSchemaSetName(schema->children[0], "col");
+    ASSERT_THAT(adbc_validation::MakeBatch<int64_t>(&schema.value, &array.value,
+                                                    &na_error, values),
+                adbc_validation::IsOkErrno());
+
+    ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error),
+                adbc_validation::IsOkStatus(&error));
+    ASSERT_THAT(AdbcStatementSetOption(&statement, ADBC_INGEST_OPTION_TARGET_TABLE,
+                                       "bulk_ingest", &error),
+                adbc_validation::IsOkStatus(&error));
+    ASSERT_THAT(AdbcStatementBind(&statement, &array.value, &schema.value, &error),
+                adbc_validation::IsOkStatus(&error));
+
+    int64_t rows_affected = 0;
+    ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, &rows_affected, &error),
+                adbc_validation::IsOkStatus(&error));
+    ASSERT_THAT(rows_affected,
+                ::testing::AnyOf(::testing::Eq(values.size()), ::testing::Eq(-1)));
+
+    ASSERT_THAT(AdbcStatementSetSqlQuery(
+                    &statement,
+                    "SELECT * FROM bulk_ingest ORDER BY \"col\" ASC NULLS FIRST", &error),
+                adbc_validation::IsOkStatus(&error));
+    {
+      adbc_validation::StreamReader reader;
+      ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
+                                            &reader.rows_affected, &error),
+                  adbc_validation::IsOkStatus(&error));
+      ASSERT_THAT(reader.rows_affected,
+                  ::testing::AnyOf(::testing::Eq(values.size()), ::testing::Eq(-1)));
+
+      ASSERT_NO_FATAL_FAILURE(reader.GetSchema());
+
+      ArrowType round_trip_type = quirks()->IngestSelectRoundTripType(type);
+      ASSERT_NO_FATAL_FAILURE(adbc_validation::CompareSchema(
+          &reader.schema.value, {{"col", round_trip_type, /*NULLABLE=*/true}}));
+
+      ASSERT_NO_FATAL_FAILURE(reader.Next());
+      ASSERT_NE(nullptr, reader.array->release);
+      ASSERT_EQ(values.size(), reader.array->length);
+      ASSERT_EQ(1, reader.array->n_children);
+
+      std::vector<std::optional<int64_t>> expected;

Review Comment:
   I think that'd be preferable over copy-pasting the entire test (effectively) into each subclass



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org