You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by li...@apache.org on 2023/06/30 15:48:30 UTC
[arrow-adbc] branch main updated: feat(c/driver_manager,go/adbc,python): trim down error messages (#866)
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 b8c98382 feat(c/driver_manager,go/adbc,python): trim down error messages (#866)
b8c98382 is described below
commit b8c98382419b10c89a57885938a7c574c7982a23
Author: David Li <li...@gmail.com>
AuthorDate: Fri Jun 30 11:48:25 2023 -0400
feat(c/driver_manager,go/adbc,python): trim down error messages (#866)
- Avoid useless prefixes and redundant information (like ADBC_ prefixes
everywhere)
- Trim out some internal bits (like exceptions technically being in
_lib)
Fixes #865.
---
c/driver_manager/adbc_driver_manager.cc | 40 ++++++++++------------
glib/test/test-connection.rb | 4 +--
.../driver/flightsql/flightsql_adbc_server_test.go | 2 ++
go/adbc/driver/flightsql/flightsql_adbc_test.go | 2 +-
go/adbc/driver/flightsql/utils.go | 6 ++--
go/adbc/drivermgr/adbc_driver_manager.cc | 40 ++++++++++------------
python/adbc_driver_flightsql/tests/test_dbapi.py | 12 +++++++
.../adbc_driver_manager/_lib.pyx | 14 ++++++++
r/adbcdrivermanager/R/error.R | 2 +-
.../tests/testthat/_snaps/driver_log.md | 3 +-
.../tests/testthat/_snaps/helpers.md | 18 ++++------
r/adbcdrivermanager/tests/testthat/test-helpers.R | 12 +++----
r/adbcdrivermanager/tests/testthat/test-radbc.R | 30 ++++++++--------
13 files changed, 100 insertions(+), 85 deletions(-)
diff --git a/c/driver_manager/adbc_driver_manager.cc b/c/driver_manager/adbc_driver_manager.cc
index c63560a4..d2929e21 100644
--- a/c/driver_manager/adbc_driver_manager.cc
+++ b/c/driver_manager/adbc_driver_manager.cc
@@ -605,34 +605,30 @@ AdbcStatusCode AdbcStatementSetSubstraitPlan(struct AdbcStatement* statement,
}
const char* AdbcStatusCodeMessage(AdbcStatusCode code) {
-#define STRINGIFY(s) #s
-#define STRINGIFY_VALUE(s) STRINGIFY(s)
-#define CASE(CONSTANT) \
- case CONSTANT: \
- return #CONSTANT " (" STRINGIFY_VALUE(CONSTANT) ")";
+#define CASE(CONSTANT) \
+ case ADBC_STATUS_##CONSTANT: \
+ return #CONSTANT;
switch (code) {
- CASE(ADBC_STATUS_OK);
- CASE(ADBC_STATUS_UNKNOWN);
- CASE(ADBC_STATUS_NOT_IMPLEMENTED);
- CASE(ADBC_STATUS_NOT_FOUND);
- CASE(ADBC_STATUS_ALREADY_EXISTS);
- CASE(ADBC_STATUS_INVALID_ARGUMENT);
- CASE(ADBC_STATUS_INVALID_STATE);
- CASE(ADBC_STATUS_INVALID_DATA);
- CASE(ADBC_STATUS_INTEGRITY);
- CASE(ADBC_STATUS_INTERNAL);
- CASE(ADBC_STATUS_IO);
- CASE(ADBC_STATUS_CANCELLED);
- CASE(ADBC_STATUS_TIMEOUT);
- CASE(ADBC_STATUS_UNAUTHENTICATED);
- CASE(ADBC_STATUS_UNAUTHORIZED);
+ CASE(OK);
+ CASE(UNKNOWN);
+ CASE(NOT_IMPLEMENTED);
+ CASE(NOT_FOUND);
+ CASE(ALREADY_EXISTS);
+ CASE(INVALID_ARGUMENT);
+ CASE(INVALID_STATE);
+ CASE(INVALID_DATA);
+ CASE(INTEGRITY);
+ CASE(INTERNAL);
+ CASE(IO);
+ CASE(CANCELLED);
+ CASE(TIMEOUT);
+ CASE(UNAUTHENTICATED);
+ CASE(UNAUTHORIZED);
default:
return "(invalid code)";
}
#undef CASE
-#undef STRINGIFY_VALUE
-#undef STRINGIFY
}
AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
diff --git a/glib/test/test-connection.rb b/glib/test/test-connection.rb
index 8706a022..13f8dc1c 100644
--- a/glib/test/test-connection.rb
+++ b/glib/test/test-connection.rb
@@ -491,7 +491,7 @@ class ConnectionTest < Test::Unit::TestCase
open_connection do |connection|
message =
"[adbc][connection][set-option]" +
- "[ADBC_STATUS_NOT_IMPLEMENTED (2)][0] " +
+ "[NOT_IMPLEMENTED][0] " +
"[SQLite] Unknown connection option adbc.connection.readonly=false"
assert_raise(ADBC::Error::NotImplemented.new(message)) do
connection.read_only = false
@@ -503,7 +503,7 @@ class ConnectionTest < Test::Unit::TestCase
open_connection do |connection|
message =
"[adbc][connection][set-option]" +
- "[ADBC_STATUS_NOT_IMPLEMENTED (2)][0] " +
+ "[NOT_IMPLEMENTED][0] " +
"[SQLite] Unknown connection option " +
"adbc.connection.transaction.isolation_level=" +
"adbc.connection.transaction.isolation.linearizable"
diff --git a/go/adbc/driver/flightsql/flightsql_adbc_server_test.go b/go/adbc/driver/flightsql/flightsql_adbc_server_test.go
index 9d959ac4..11c5c31e 100644
--- a/go/adbc/driver/flightsql/flightsql_adbc_server_test.go
+++ b/go/adbc/driver/flightsql/flightsql_adbc_server_test.go
@@ -333,6 +333,8 @@ func (ts *TimeoutTests) TestDoActionTimeout() {
var adbcErr adbc.Error
ts.ErrorAs(stmt.Prepare(context.Background()), &adbcErr)
ts.Equal(adbc.StatusTimeout, adbcErr.Code, adbcErr.Error())
+ // Exact match - we don't want extra fluff in the message
+ ts.Equal("context deadline exceeded", adbcErr.Msg)
}
func (ts *TimeoutTests) TestDoGetTimeout() {
diff --git a/go/adbc/driver/flightsql/flightsql_adbc_test.go b/go/adbc/driver/flightsql/flightsql_adbc_test.go
index 53dbac24..2f960934 100644
--- a/go/adbc/driver/flightsql/flightsql_adbc_test.go
+++ b/go/adbc/driver/flightsql/flightsql_adbc_test.go
@@ -854,7 +854,7 @@ func (suite *TLSTests) TestInvalidOptions() {
suite.NoError(stmt.SetSqlQuery("SELECT 1"))
_, _, err = stmt.ExecuteQuery(suite.ctx)
- suite.Contains(err.Error(), "Unavailable")
+ suite.Contains(err.Error(), "connection error")
}
type ConnectionTests struct {
diff --git a/go/adbc/driver/flightsql/utils.go b/go/adbc/driver/flightsql/utils.go
index 4f1f165c..cbf9048f 100644
--- a/go/adbc/driver/flightsql/utils.go
+++ b/go/adbc/driver/flightsql/utils.go
@@ -29,7 +29,9 @@ func adbcFromFlightStatus(err error) error {
}
var adbcCode adbc.Status
- switch status.Code(err) {
+ // If not a status.Status, will return codes.Unknown
+ grpcStatus := status.Convert(err)
+ switch grpcStatus.Code() {
case codes.OK:
return nil
case codes.Canceled:
@@ -69,7 +71,7 @@ func adbcFromFlightStatus(err error) error {
}
return adbc.Error{
- Msg: err.Error(),
+ Msg: grpcStatus.Message(),
Code: adbcCode,
}
}
diff --git a/go/adbc/drivermgr/adbc_driver_manager.cc b/go/adbc/drivermgr/adbc_driver_manager.cc
index c63560a4..d2929e21 100644
--- a/go/adbc/drivermgr/adbc_driver_manager.cc
+++ b/go/adbc/drivermgr/adbc_driver_manager.cc
@@ -605,34 +605,30 @@ AdbcStatusCode AdbcStatementSetSubstraitPlan(struct AdbcStatement* statement,
}
const char* AdbcStatusCodeMessage(AdbcStatusCode code) {
-#define STRINGIFY(s) #s
-#define STRINGIFY_VALUE(s) STRINGIFY(s)
-#define CASE(CONSTANT) \
- case CONSTANT: \
- return #CONSTANT " (" STRINGIFY_VALUE(CONSTANT) ")";
+#define CASE(CONSTANT) \
+ case ADBC_STATUS_##CONSTANT: \
+ return #CONSTANT;
switch (code) {
- CASE(ADBC_STATUS_OK);
- CASE(ADBC_STATUS_UNKNOWN);
- CASE(ADBC_STATUS_NOT_IMPLEMENTED);
- CASE(ADBC_STATUS_NOT_FOUND);
- CASE(ADBC_STATUS_ALREADY_EXISTS);
- CASE(ADBC_STATUS_INVALID_ARGUMENT);
- CASE(ADBC_STATUS_INVALID_STATE);
- CASE(ADBC_STATUS_INVALID_DATA);
- CASE(ADBC_STATUS_INTEGRITY);
- CASE(ADBC_STATUS_INTERNAL);
- CASE(ADBC_STATUS_IO);
- CASE(ADBC_STATUS_CANCELLED);
- CASE(ADBC_STATUS_TIMEOUT);
- CASE(ADBC_STATUS_UNAUTHENTICATED);
- CASE(ADBC_STATUS_UNAUTHORIZED);
+ CASE(OK);
+ CASE(UNKNOWN);
+ CASE(NOT_IMPLEMENTED);
+ CASE(NOT_FOUND);
+ CASE(ALREADY_EXISTS);
+ CASE(INVALID_ARGUMENT);
+ CASE(INVALID_STATE);
+ CASE(INVALID_DATA);
+ CASE(INTEGRITY);
+ CASE(INTERNAL);
+ CASE(IO);
+ CASE(CANCELLED);
+ CASE(TIMEOUT);
+ CASE(UNAUTHENTICATED);
+ CASE(UNAUTHORIZED);
default:
return "(invalid code)";
}
#undef CASE
-#undef STRINGIFY_VALUE
-#undef STRINGIFY
}
AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
diff --git a/python/adbc_driver_flightsql/tests/test_dbapi.py b/python/adbc_driver_flightsql/tests/test_dbapi.py
index cf72ab0e..0918fc7a 100644
--- a/python/adbc_driver_flightsql/tests/test_dbapi.py
+++ b/python/adbc_driver_flightsql/tests/test_dbapi.py
@@ -16,11 +16,23 @@
# under the License.
import pyarrow
+import pytest
import adbc_driver_flightsql.dbapi
import adbc_driver_manager
+def test_query_error(dremio_dbapi):
+ with dremio_dbapi.cursor() as cur:
+ with pytest.raises(adbc_driver_flightsql.dbapi.ProgrammingError) as exc_info:
+ cur.execute("SELECT")
+
+ exc = exc_info.value
+ assert exc.status_code == adbc_driver_manager.AdbcStatusCode.INVALID_ARGUMENT
+ # Try to keep noise in exceptions minimal
+ assert exc.args[0].startswith("INVALID_ARGUMENT: [FlightSQL] ")
+
+
def test_query_trivial(dremio_dbapi):
with dremio_dbapi.cursor() as cur:
cur.execute("SELECT 1")
diff --git a/python/adbc_driver_manager/adbc_driver_manager/_lib.pyx b/python/adbc_driver_manager/adbc_driver_manager/_lib.pyx
index 2e8658fc..406d5778 100644
--- a/python/adbc_driver_manager/adbc_driver_manager/_lib.pyx
+++ b/python/adbc_driver_manager/adbc_driver_manager/_lib.pyx
@@ -331,6 +331,20 @@ class NotSupportedError(DatabaseError):
)
+# XXX: shorten the traceback a bit (and avoid exposing _lib). We
+# could also define the exceptions in __init__ but then we'd have a
+# circular import situation
+Error.__module__ = "adbc_driver_manager"
+InterfaceError.__module__ = "adbc_driver_manager"
+DatabaseError.__module__ = "adbc_driver_manager"
+DataError.__module__ = "adbc_driver_manager"
+OperationalError.__module__ = "adbc_driver_manager"
+IntegrityError.__module__ = "adbc_driver_manager"
+InternalError.__module__ = "adbc_driver_manager"
+ProgrammingError.__module__ = "adbc_driver_manager"
+NotSupportedError.__module__ = "adbc_driver_manager"
+
+
INGEST_OPTION_MODE = ADBC_INGEST_OPTION_MODE.decode("utf-8")
INGEST_OPTION_MODE_APPEND = ADBC_INGEST_OPTION_MODE_APPEND.decode("utf-8")
INGEST_OPTION_MODE_CREATE = ADBC_INGEST_OPTION_MODE_CREATE.decode("utf-8")
diff --git a/r/adbcdrivermanager/R/error.R b/r/adbcdrivermanager/R/error.R
index 8187a55b..e87e4eae 100644
--- a/r/adbcdrivermanager/R/error.R
+++ b/r/adbcdrivermanager/R/error.R
@@ -29,7 +29,7 @@ stop_for_error <- function(status, error) {
# Gives an error class like "adbc_status_invalid_state", "adbc_status",
# "simpleError", ...
cnd_class <- c(
- tolower(gsub("\\s+.*", "", error$status_code_message)),
+ paste0("adbc_status_", tolower(gsub("\\s+.*", "", error$status_code_message))),
"adbc_status"
)
diff --git a/r/adbcdrivermanager/tests/testthat/_snaps/driver_log.md b/r/adbcdrivermanager/tests/testthat/_snaps/driver_log.md
index fdbbe938..74527250 100644
--- a/r/adbcdrivermanager/tests/testthat/_snaps/driver_log.md
+++ b/r/adbcdrivermanager/tests/testthat/_snaps/driver_log.md
@@ -21,8 +21,7 @@
try(adbc_statement_execute_query(stmt))
Output
LogStatementExecuteQuery()
- Error in adbc_statement_execute_query(stmt) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_execute_query(stmt) : NOT_IMPLEMENTED
Code
adbc_statement_release(stmt)
Output
diff --git a/r/adbcdrivermanager/tests/testthat/_snaps/helpers.md b/r/adbcdrivermanager/tests/testthat/_snaps/helpers.md
index 5b758cfa..429b0fe8 100644
--- a/r/adbcdrivermanager/tests/testthat/_snaps/helpers.md
+++ b/r/adbcdrivermanager/tests/testthat/_snaps/helpers.md
@@ -14,8 +14,7 @@
LogStatementSetSqlQuery()
LogStatementRelease()
LogConnectionRelease()
- Error in adbc_statement_set_sql_query(stmt, query) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_set_sql_query(stmt, query) : NOT_IMPLEMENTED
Code
try(execute_adbc(db, "some sql"))
Output
@@ -25,8 +24,7 @@
LogStatementSetSqlQuery()
LogStatementRelease()
LogConnectionRelease()
- Error in adbc_statement_set_sql_query(stmt, query) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_set_sql_query(stmt, query) : NOT_IMPLEMENTED
Code
try(write_adbc(mtcars, db, "some_table"))
Output
@@ -37,8 +35,7 @@
LogStatementBindStream()
LogStatementRelease()
LogConnectionRelease()
- Error in adbc_statement_bind_stream(stmt, tbl) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_bind_stream(stmt, tbl) : NOT_IMPLEMENTED
Code
adbc_database_release(db)
Output
@@ -63,16 +60,14 @@
LogStatementNew()
LogStatementSetSqlQuery()
LogStatementRelease()
- Error in adbc_statement_set_sql_query(stmt, query) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_set_sql_query(stmt, query) : NOT_IMPLEMENTED
Code
try(execute_adbc(con, "some sql"))
Output
LogStatementNew()
LogStatementSetSqlQuery()
LogStatementRelease()
- Error in adbc_statement_set_sql_query(stmt, query) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_set_sql_query(stmt, query) : NOT_IMPLEMENTED
Code
try(write_adbc(mtcars, con, "some_table"))
Output
@@ -80,8 +75,7 @@
LogStatementSetOption()
LogStatementBindStream()
LogStatementRelease()
- Error in adbc_statement_bind_stream(stmt, tbl) :
- ADBC_STATUS_NOT_IMPLEMENTED (2)
+ Error in adbc_statement_bind_stream(stmt, tbl) : NOT_IMPLEMENTED
Code
adbc_connection_release(con)
Output
diff --git a/r/adbcdrivermanager/tests/testthat/test-helpers.R b/r/adbcdrivermanager/tests/testthat/test-helpers.R
index 9096a42d..3e65053e 100644
--- a/r/adbcdrivermanager/tests/testthat/test-helpers.R
+++ b/r/adbcdrivermanager/tests/testthat/test-helpers.R
@@ -44,7 +44,7 @@ test_that("with_adbc() and local_adbc() release databases", {
expect_identical(with_adbc(db, "value"), "value")
expect_error(
adbc_database_release(db),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
db <- adbc_database_init(adbc_driver_void())
@@ -53,7 +53,7 @@ test_that("with_adbc() and local_adbc() release databases", {
})
expect_error(
adbc_database_release(db),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
})
@@ -63,7 +63,7 @@ test_that("with_adbc() and local_adbc() release connections", {
expect_identical(with_adbc(con, "value"), "value")
expect_error(
adbc_connection_release(con),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
con <- adbc_connection_init(db)
@@ -72,7 +72,7 @@ test_that("with_adbc() and local_adbc() release connections", {
})
expect_error(
adbc_connection_release(con),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
})
@@ -83,7 +83,7 @@ test_that("with_adbc() and local_adbc() release statements", {
expect_identical(with_adbc(stmt, "value"), "value")
expect_error(
adbc_statement_release(stmt),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
stmt <- adbc_statement_init(con)
@@ -92,7 +92,7 @@ test_that("with_adbc() and local_adbc() release statements", {
})
expect_error(
adbc_statement_release(stmt),
- "ADBC_STATUS_INVALID_STATE"
+ "INVALID_STATE"
)
})
diff --git a/r/adbcdrivermanager/tests/testthat/test-radbc.R b/r/adbcdrivermanager/tests/testthat/test-radbc.R
index f7e7eff8..e7aa4bda 100644
--- a/r/adbcdrivermanager/tests/testthat/test-radbc.R
+++ b/r/adbcdrivermanager/tests/testthat/test-radbc.R
@@ -19,7 +19,7 @@ test_that("can initialize and release a database", {
db <- adbc_database_init(adbc_driver_void(), some_key = "some_value")
expect_s3_class(db, "adbc_database")
adbc_database_release(db)
- expect_error(adbc_database_release(db), "ADBC_STATUS_INVALID_STATE")
+ expect_error(adbc_database_release(db), "INVALID_STATE")
})
test_that("can initialize and release a connection", {
@@ -27,7 +27,7 @@ test_that("can initialize and release a connection", {
con <- adbc_connection_init(db, some_key = "some_value")
expect_s3_class(con, "adbc_connection")
adbc_connection_release(con)
- expect_error(adbc_connection_release(con), "ADBC_STATUS_INVALID_STATE")
+ expect_error(adbc_connection_release(con), "INVALID_STATE")
})
test_that("connection methods work for the void driver", {
@@ -36,7 +36,7 @@ test_that("connection methods work for the void driver", {
expect_error(
adbc_connection_get_info(con, integer()),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
@@ -45,7 +45,7 @@ test_that("connection methods work for the void driver", {
"catalog", "db_schema",
"table_name", "table_type", "column_name"
),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
@@ -53,17 +53,17 @@ test_that("connection methods work for the void driver", {
con,
"catalog", "db_schema", "table_name"
),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_connection_get_table_types(con),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_connection_read_partition(con, raw()),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_identical(
@@ -83,7 +83,7 @@ test_that("can initialize and release a statement", {
stmt <- adbc_statement_init(con, some_key = "some_value")
expect_s3_class(stmt, "adbc_statement")
adbc_statement_release(stmt)
- expect_error(adbc_statement_release(stmt), "ADBC_STATUS_INVALID_STATE")
+ expect_error(adbc_statement_release(stmt), "INVALID_STATE")
})
test_that("statement methods work for the void driver", {
@@ -93,38 +93,38 @@ test_that("statement methods work for the void driver", {
expect_error(
adbc_statement_set_sql_query(stmt, "some query"),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_statement_set_substrait_plan(stmt, charToRaw("some plan")),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_statement_prepare(stmt),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_statement_get_parameter_schema(stmt),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
struct_array <- nanoarrow::as_nanoarrow_array(data.frame(x = 1:5))
expect_error(
adbc_statement_bind(stmt, struct_array),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_statement_bind_stream(stmt, nanoarrow::nanoarrow_allocate_array_stream()),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
expect_error(
adbc_statement_execute_query(stmt),
- "ADBC_STATUS_NOT_IMPLEMENTED"
+ "NOT_IMPLEMENTED"
)
})