Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(c): Changed import to common/util #783

Merged
merged 10 commits into from
Jun 14, 2023
Merged

chore(c): Changed import to common/util #783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4e8952a
Changed import to common/util
WillAyd Jun 13, 2023
201edd9
remove SYSTEM include directive
WillAyd Jun 13, 2023
25542d5
make clang-format happy
WillAyd Jun 13, 2023
1c9f254
try R fix
WillAyd Jun 13, 2023
0e85581
fixed driver_manager CMake
WillAyd Jun 13, 2023
9e7b3eb
try more R fix
WillAyd Jun 13, 2023
46cf086
fix R issues
WillAyd Jun 13, 2023
983c8b0
R postgres fixup
WillAyd Jun 13, 2023
0a39371
More Windows R compat
WillAyd Jun 13, 2023
db189d8
more R compat?
WillAyd Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions c/driver/common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

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)
target_include_directories(adbc_driver_common SYSTEM
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason for us to specify SYSTEM here? Just repeating what I see on other includes but not sure the purpose

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want SYSTEM for this one; SYSTEM translates to -isystem or the compiler equivalent which basically means to not issue warnings for things in that header.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for nanoarrow I wanted the SYSTEM flag for precisely that reason (that said, we could probably also include nanoarrow as non-system)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice. Didn't realize that was the behavior with -isystem includes

PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/vendor")

if(ADBC_BUILD_TESTS)
add_test_case(driver_common_test
Expand All @@ -32,5 +32,7 @@ if(ADBC_BUILD_TESTS)
adbc_driver_common
nanoarrow)
target_compile_features(adbc-driver-common-test PRIVATE cxx_std_17)
target_include_directories(adbc-driver-common-test
PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/vendor")
adbc_configure_target(adbc-driver-common-test)
endif()
2 changes: 1 addition & 1 deletion c/driver/flightsql/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ add_go_lib("${REPOSITORY_ROOT}/go/adbc/pkg/flightsql/"

include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)

if(ADBC_TEST_LINKAGE STREQUAL "shared")
Expand All @@ -42,7 +43,6 @@ else()
endif()

if(ADBC_BUILD_TESTS)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver/common)
add_test_case(driver_flightsql_test
PREFIX
adbc
Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${LIBPQ_INCLUDE_DIRS})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver/common)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver)

foreach(LIB_TARGET ${ADBC_LIBRARIES})
target_compile_definitions(${LIB_TARGET} PRIVATE ADBC_EXPORTING)
Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
#include <adbc.h>
#include <libpq-fe.h>

#include "common/utils.h"
#include "database.h"
#include "utils.h"

namespace {

Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/database.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include <libpq-fe.h>
#include <nanoarrow/nanoarrow.h>

#include "utils.h"
#include "common/utils.h"

namespace adbcpq {

Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/postgresql.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@

#include <adbc.h>

#include "common/utils.h"
#include "connection.h"
#include "database.h"
#include "statement.h"
#include "utils.h"

using adbcpq::PostgresConnection;
using adbcpq::PostgresDatabase;
Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/postgresql_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include <gtest/gtest-param-test.h>
#include <gtest/gtest.h>
#include <nanoarrow/nanoarrow.h>
#include <utils.h>
#include "common/utils.h"

#include "validation/adbc_validation.h"
#include "validation/adbc_validation_util.h"
Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/statement.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@
#include <libpq-fe.h>
#include <nanoarrow/nanoarrow.hpp>

#include "common/utils.h"
#include "connection.h"
#include "postgres_copy_reader.h"
#include "postgres_type.h"
#include "postgres_util.h"
#include "utils.h"

namespace adbcpq {

Expand Down
2 changes: 1 addition & 1 deletion c/driver/postgresql/statement.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
#include <libpq-fe.h>
#include <nanoarrow/nanoarrow.h>

#include "common/utils.h"
#include "postgres_copy_reader.h"
#include "postgres_type.h"
#include "utils.h"

namespace adbcpq {
class PostgresConnection;
Expand Down
1 change: 1 addition & 0 deletions c/driver/snowflake/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ add_go_lib("${REPOSITORY_ROOT}/go/adbc/pkg/snowflake/"

include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)

if(ADBC_TEST_LINKAGE STREQUAL "shared")
Expand Down
2 changes: 1 addition & 1 deletion c/driver/sqlite/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ add_arrow_lib(adbc_driver_sqlite
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver/common)
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/driver)
include_directories(SYSTEM ${SQLite3_INCLUDE_DIRS})

foreach(LIB_TARGET ${ADBC_LIBRARIES})
Expand Down
2 changes: 1 addition & 1 deletion c/driver/sqlite/sqlite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@
#include <nanoarrow/nanoarrow.h>
#include <sqlite3.h>

#include "common/utils.h"
#include "statement_reader.h"
#include "types.h"
#include "utils.h"

static const char kDefaultUri[] = "file:adbc_driver_sqlite?mode=memory&cache=shared";
// The batch size for query results (and for initial type inference)
Expand Down
2 changes: 1 addition & 1 deletion c/driver/sqlite/statement_reader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include <nanoarrow/nanoarrow.h>
#include <sqlite3.h>

#include "utils.h"
#include "common/utils.h"

AdbcStatusCode AdbcSqliteBinderSet(struct AdbcSqliteBinder* binder,
struct AdbcError* error) {
Expand Down
3 changes: 1 addition & 2 deletions c/validation/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
add_library(adbc_validation OBJECT adbc_validation.cc adbc_validation_util.cc)
target_compile_features(adbc_validation PRIVATE cxx_std_17)
target_include_directories(adbc_validation SYSTEM
PRIVATE "${REPOSITORY_ROOT}"
"${REPOSITORY_ROOT}/c/driver/common"
PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/driver/"
"${REPOSITORY_ROOT}/c/vendor/")
target_link_libraries(adbc_validation PUBLIC nanoarrow GTest::gtest GTest::gmock)
10 changes: 3 additions & 7 deletions c/validation/adbc_validation_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <nanoarrow/nanoarrow.h>
#include "utils.h"
#include "common/utils.h"

namespace adbc_validation {

Expand Down Expand Up @@ -206,12 +206,8 @@ struct GetObjectsReader {
}
~GetObjectsReader() { AdbcGetObjectsDataDelete(get_objects_data_); }

struct AdbcGetObjectsData* operator*() {
return get_objects_data_;
}
struct AdbcGetObjectsData* operator->() {
return get_objects_data_;
}
struct AdbcGetObjectsData* operator*() { return get_objects_data_; }
struct AdbcGetObjectsData* operator->() { return get_objects_data_; }

private:
struct ArrowArrayView* array_view_;
Expand Down
11 changes: 7 additions & 4 deletions c/vendor/nanoarrow/nanoarrow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ static inline void ArrowArrayStreamMove(struct ArrowArrayStream* src,
#define _NANOARROW_CHECK_RANGE(x_, min_, max_) \
NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL)

#define _NANOARROW_CHECK_UPPER_LIMIT(x_, max_) \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this was getting errors like:

/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:192:31: error: comparison of unsigned expression in ‘>= 0’ is always true [-Werror=type-limits]
  192 |   NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL)
      |                               ^
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:187:23: note: in definition of macro ‘_NANOARROW_RETURN_NOT_OK_IMPL’
  187 |     const int NAME = (EXPR);                      \
      |                       ^~~~
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:192:3: note: in expansion of macro ‘NANOARROW_RETURN_NOT_OK’
  192 |   NANOARROW_RETURN_NOT_OK((x_ >= min_ && x_ <= max_) ? NANOARROW_OK : EINVAL)
      |   ^~~~~~~~~~~~~~~~~~~~~~~
/home/willayd/clones/arrow-adbc/c/vendor/nanoarrow/nanoarrow.h:2606:7: note: in expansion of macro ‘_NANOARROW_CHECK_RANGE’
 2606 |       _NANOARROW_CHECK_RANGE(value, 0, UINT8_MAX);

which make sense. Not sure why they are appearing now. Need to upstream this in nanoarrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref apache/arrow-nanoarrow#233

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your other comment here explains why this wasn't producing warnings/errors before

NANOARROW_RETURN_NOT_OK((x_ <= max_) ? NANOARROW_OK : EINVAL)

/// \brief Return code for success.
/// \ingroup nanoarrow-errors
#define NANOARROW_OK 0
Expand Down Expand Up @@ -2595,22 +2598,22 @@ static inline ArrowErrorCode ArrowArrayAppendUInt(struct ArrowArray* array,
NANOARROW_RETURN_NOT_OK(ArrowBufferAppend(data_buffer, &value, sizeof(uint64_t)));
break;
case NANOARROW_TYPE_UINT32:
_NANOARROW_CHECK_RANGE(value, 0, UINT32_MAX);
_NANOARROW_CHECK_UPPER_LIMIT(value, UINT32_MAX);
NANOARROW_RETURN_NOT_OK(ArrowBufferAppendUInt32(data_buffer, (uint32_t)value));
break;
case NANOARROW_TYPE_UINT16:
_NANOARROW_CHECK_RANGE(value, 0, UINT16_MAX);
_NANOARROW_CHECK_UPPER_LIMIT(value, UINT16_MAX);
NANOARROW_RETURN_NOT_OK(ArrowBufferAppendUInt16(data_buffer, (uint16_t)value));
break;
case NANOARROW_TYPE_UINT8:
_NANOARROW_CHECK_RANGE(value, 0, UINT8_MAX);
_NANOARROW_CHECK_UPPER_LIMIT(value, UINT8_MAX);
NANOARROW_RETURN_NOT_OK(ArrowBufferAppendUInt8(data_buffer, (uint8_t)value));
break;
case NANOARROW_TYPE_INT64:
case NANOARROW_TYPE_INT32:
case NANOARROW_TYPE_INT16:
case NANOARROW_TYPE_INT8:
_NANOARROW_CHECK_RANGE(value, 0, INT64_MAX);
_NANOARROW_CHECK_UPPER_LIMIT(value, INT64_MAX);
return ArrowArrayAppendInt(array, value);
case NANOARROW_TYPE_DOUBLE:
NANOARROW_RETURN_NOT_OK(ArrowBufferAppendDouble(data_buffer, value));
Expand Down