test(c): don't use sketchy cast to test backwards compatibility (#2425)

- Backport nanoarrow patch to satisfy newer Clang - Add test using GCC 15 - Update tests using sketchy casts to satisfy these compilers - Refactor the clang/gcc Docker jobs Fixes #2424. --------- Co-authored-by: Sutou Kouhei <[email protected]>
apache · Jan 9, 2025 · 63bb903 · 63bb903
1 parent 196522b
commit 63bb903
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 46 deletions.
diff --git a/.github/workflows/nightly-verify.yml b/.github/workflows/nightly-verify.yml
@@ -190,6 +190,11 @@ jobs:
           pushd arrow-adbc
           docker compose run --rm cpp-clang-latest
 
+      - name: cpp-gcc-latest
+        run: |
+          pushd arrow-adbc
+          docker compose run --rm cpp-gcc-latest
+
       - name: python-debug
         run: |
           pushd arrow-adbc

diff --git a/c/driver/flightsql/sqlite_flightsql_test.cc b/c/driver/flightsql/sqlite_flightsql_test.cc
@@ -235,18 +235,20 @@ TEST_F(SqliteFlightSqlTest, TestGarbageInput) {
   ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
 }
 
+int Canary(const struct AdbcError*) { return 0; }
+
 TEST_F(SqliteFlightSqlTest, AdbcDriverBackwardsCompatibility) {
-  // XXX: sketchy cast
-  auto* driver = static_cast<struct AdbcDriver*>(malloc(ADBC_DRIVER_1_0_0_SIZE));
-  std::memset(driver, 0, ADBC_DRIVER_1_0_0_SIZE);
+  struct AdbcDriver driver;
+  std::memset(&driver, 0, ADBC_DRIVER_1_1_0_SIZE);
+  driver.ErrorGetDetailCount = Canary;
 
-  ASSERT_THAT(::FlightSQLDriverInit(ADBC_VERSION_1_0_0, driver, &error),
+  ASSERT_THAT(::FlightSQLDriverInit(ADBC_VERSION_1_0_0, &driver, &error),
               IsOkStatus(&error));
 
-  ASSERT_THAT(::FlightSQLDriverInit(424242, driver, &error),
-              adbc_validation::IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error));
+  ASSERT_EQ(Canary, driver.ErrorGetDetailCount);
 
-  free(driver);
+  ASSERT_THAT(::FlightSQLDriverInit(424242, &driver, &error),
+              adbc_validation::IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error));
 }
 
 class SqliteFlightSqlConnectionTest : public ::testing::Test,

diff --git a/c/driver/postgresql/postgresql_test.cc b/c/driver/postgresql/postgresql_test.cc
@@ -223,18 +223,20 @@ class PostgresDatabaseTest : public ::testing::Test,
 };
 ADBCV_TEST_DATABASE(PostgresDatabaseTest)
 
+int Canary(const struct AdbcError*) { return 0; }
+
 TEST_F(PostgresDatabaseTest, AdbcDriverBackwardsCompatibility) {
-  // XXX: sketchy cast
-  auto* driver = static_cast<struct AdbcDriver*>(malloc(ADBC_DRIVER_1_0_0_SIZE));
-  std::memset(driver, 0, ADBC_DRIVER_1_0_0_SIZE);
+  struct AdbcDriver driver;
+  std::memset(&driver, 0, ADBC_DRIVER_1_1_0_SIZE);
+  driver.ErrorGetDetailCount = Canary;
 
-  ASSERT_THAT(::PostgresqlDriverInit(ADBC_VERSION_1_0_0, driver, &error),
+  ASSERT_THAT(::PostgresqlDriverInit(ADBC_VERSION_1_0_0, &driver, &error),
               IsOkStatus(&error));
 
-  ASSERT_THAT(::PostgresqlDriverInit(424242, driver, &error),
-              IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error));
+  ASSERT_EQ(Canary, driver.ErrorGetDetailCount);
 
-  free(driver);
+  ASSERT_THAT(::PostgresqlDriverInit(424242, &driver, &error),
+              IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error));
 }
 
 class PostgresConnectionTest : public ::testing::Test,
@@ -1552,24 +1554,25 @@ TEST_F(PostgresStatementTest, BatchSizeHint) {
 
 // Test that an ADBC 1.0.0-sized error still works
 TEST_F(PostgresStatementTest, AdbcErrorBackwardsCompatibility) {
-  // XXX: sketchy cast
-  auto* error = static_cast<struct AdbcError*>(malloc(ADBC_ERROR_1_0_0_SIZE));
-  std::memset(error, 0, ADBC_ERROR_1_0_0_SIZE);
+  struct AdbcError error;
+  std::memset(&error, 0, ADBC_ERROR_1_1_0_SIZE);
+  struct AdbcDriver canary;
+  error.private_data = &canary;
+  error.private_driver = &canary;
 
-  ASSERT_THAT(AdbcStatementNew(&connection, &statement, error), IsOkStatus(error));
+  ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
   ASSERT_THAT(
-      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", error),
-      IsOkStatus(error));
+      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", &error),
+      IsOkStatus(&error));
   adbc_validation::StreamReader reader;
   ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
-                                        &reader.rows_affected, error),
-              IsStatus(ADBC_STATUS_NOT_FOUND, error));
-
-  ASSERT_EQ("42P01", std::string_view(error->sqlstate, 5));
-  ASSERT_EQ(0, AdbcErrorGetDetailCount(error));
-
-  error->release(error);
-  free(error);
+                                        &reader.rows_affected, &error),
+              IsStatus(ADBC_STATUS_NOT_FOUND, &error));
+  ASSERT_EQ("42P01", std::string_view(error.sqlstate, 5));
+  ASSERT_EQ(0, AdbcErrorGetDetailCount(&error));
+  ASSERT_EQ(&canary, error.private_data);
+  ASSERT_EQ(&canary, error.private_driver);
+  error.release(&error);
 }
 
 TEST_F(PostgresStatementTest, Cancel) {

diff --git a/c/validation/adbc_validation_statement.cc b/c/validation/adbc_validation_statement.cc
@@ -2817,21 +2817,23 @@ struct ADBC_EXPORT AdbcError100 {
 // Test that an ADBC 1.0.0-sized error still works
 void StatementTest::TestErrorCompatibility() {
   static_assert(sizeof(AdbcError100) == ADBC_ERROR_1_0_0_SIZE, "Wrong size");
-  // XXX: sketchy cast
-  auto* error = reinterpret_cast<struct AdbcError*>(malloc(ADBC_ERROR_1_0_0_SIZE));
-  std::memset(error, 0, ADBC_ERROR_1_0_0_SIZE);
+  struct AdbcError error;
+  std::memset(&error, 0, ADBC_ERROR_1_1_0_SIZE);
+  struct AdbcDriver canary;
+  error.private_data = &canary;
+  error.private_driver = &canary;
 
-  ASSERT_THAT(AdbcStatementNew(&connection, &statement, error), IsOkStatus(error));
+  ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
   ASSERT_THAT(
-      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", error),
-      IsOkStatus(error));
+      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", &error),
+      IsOkStatus(&error));
   adbc_validation::StreamReader reader;
   ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
-                                        &reader.rows_affected, error),
-              ::testing::Not(IsOkStatus(error)));
-  auto* old_error = reinterpret_cast<AdbcError100*>(error);
-  old_error->release(old_error);
-  free(error);
+                                        &reader.rows_affected, &error),
+              ::testing::Not(IsOkStatus(&error)));
+  ASSERT_EQ(&canary, error.private_data);
+  ASSERT_EQ(&canary, error.private_driver);
+  error.release(&error);
 }
 
 void StatementTest::TestResultInvalidation() {

diff --git a/c/vendor/nanoarrow/nanoarrow.hpp b/c/vendor/nanoarrow/nanoarrow.hpp
@@ -92,9 +92,16 @@ namespace literals {
 /// @{
 
 /// \brief User literal operator allowing ArrowStringView construction like "str"_asv
+#if !defined(__clang__) && (defined(__GNUC__) && __GNUC__ < 6)
 inline ArrowStringView operator"" _asv(const char* data, std::size_t size_bytes) {
   return {data, static_cast<int64_t>(size_bytes)};
 }
+#else
+inline ArrowStringView operator""_asv(const char* data, std::size_t size_bytes) {
+  return {data, static_cast<int64_t>(size_bytes)};
+}
+#endif
+// N.B. older GCC requires the space above, newer Clang forbids the space
 
 // @}
 

diff --git a/ci/docker/cpp-clang-latest.dockerfile b/ci/docker/cpp-clang-latest.dockerfile
@@ -15,9 +15,8 @@
 # specific language governing permissions and limitations
 # under the License.
 
-ARG VCPKG
-
 FROM debian:12
+ARG GO
 
 RUN export DEBIAN_FRONTEND=noninteractive && \
     apt-get update -y && \
@@ -34,6 +33,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 RUN export DEBIAN_FRONTEND=noninteractive && \
     apt-get install -y cmake git libpq-dev libsqlite3-dev pkg-config
 
-RUN curl -L -o go.tar.gz https://go.dev/dl/go1.22.5.linux-amd64.tar.gz && \
-    tar -C /opt -xvf go.tar.gz && \
-    echo 'export PATH=$PATH:/opt/go/bin' | tee -a ~/.bashrc
+RUN curl -L -o go.tar.gz https://go.dev/dl/go${GO}.linux-amd64.tar.gz && \
+    tar -C /opt -xvf go.tar.gz
+
+ENV PATH=/opt/go/bin:$PATH \
+    CC=/usr/bin/clang \
+    CXX=/usr/bin/clang++
diff --git a/ci/docker/cpp-gcc-latest.dockerfile b/ci/docker/cpp-gcc-latest.dockerfile
@@ -0,0 +1,34 @@
+# 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.
+
+FROM amd64/debian:experimental
+ARG GCC
+ARG GO
+
+ENV DEBIAN_FRONTEND noninteractive
+
+RUN apt-get update -y && \
+    apt-get install -y -q cmake curl git gnupg libpq-dev libsqlite3-dev pkg-config && \
+    apt-get install -y -q -t experimental g++-${GCC} gcc-${GCC} && \
+    apt-get clean
+
+RUN curl -L -o go.tar.gz https://go.dev/dl/go${GO}.linux-amd64.tar.gz && \
+    tar -C /opt -xvf go.tar.gz
+
+ENV PATH=/opt/go/bin:$PATH \
+    CC=/usr/bin/gcc-${GCC} \
+    CXX=/usr/bin/g++-${GCC}
diff --git a/docker-compose.yml b/docker-compose.yml
@@ -38,10 +38,21 @@ services:
       context: .
       dockerfile: ci/docker/cpp-clang-latest.dockerfile
       args:
-        VCPKG: ${VCPKG}
+        GO: ${GO}
+    volumes:
+      - .:/adbc:delegated
+    command: "bash -c 'git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build/clang-latest && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build/clang-latest'"
+
+  cpp-gcc-latest:
+    build:
+      context: .
+      dockerfile: ci/docker/cpp-gcc-latest.dockerfile
+      args:
+        GCC: 15
+        GO: ${GO}
     volumes:
       - .:/adbc:delegated
-    command: "bash -c 'export PATH=$PATH:/opt/go/bin CC=$(which clang) CXX=$(which clang++) && git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build'"
+    command: "bash -c 'git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build/gcc-latest && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build/gcc-latest'"
 
   ############################ Documentation ###################################