Skip to content

feat(format): add AdbcConnectionSetWarningHandler #3872

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

Merged
lidavidm merged 2 commits into from
Feb 25, 2026
Merged

feat(format): add AdbcConnectionSetWarningHandler #3872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31aa145
feat(format): add AdbcConnectionSetWarningHandler
lidavidm Jan 9, 2026
c2d6cf5
tweak wording
lidavidm Feb 23, 2026
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
48 changes: 46 additions & 2 deletions c/include/arrow-adbc/adbc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,8 +989,6 @@ struct AdbcPartitions {

/// @}

/// @}

/// \defgroup adbc-statement-multi Multiple Result Set Execution
/// Some databases support executing a statement that returns multiple
/// result sets. This section defines the API for working with such
Expand Down Expand Up @@ -1092,6 +1090,23 @@ AdbcStatusCode AdbcMultiResultSetNextPartitions(struct AdbcMultiResultSet* resul
struct AdbcError* error);
/// @}

/// \brief A warning handler function.
///
/// The handler must not block and must not call any ADBC functions (besides
/// releasing the warning). The warning does not need to be released before
/// returning, but the warning pointer itself may not be valid after the
/// handler returns.
///
/// There are no requirements on ordering or concurrency of calls to the
/// handler; the driver may call the handler at any time from any thread,
/// including calling the handler concurrently.
///
/// \param[in] warning The warning information. The application is
/// responsible for releasing the warning, but the warning pointer itself
/// may not be valid after the handler returns.
/// \param[in] user_data The user_data pointer.
typedef void (*AdbcWarningHandler)(const struct AdbcError* warning, void* user_data);
Comment on lines +1104 to +1108
Copy link
Member

@paleolimbot paleolimbot Feb 22, 2026

Choose a reason for hiding this comment

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

I think this is simpler and the right way to go, but just pointing out that it implies that the lifecycle of user_data must outlive the connection (i.e., if you want to do something like log to a file you're managing that file handle on your own).


/// \defgroup adbc-driver Driver Initialization
///
/// These functions are intended to help support integration between a
Expand Down Expand Up @@ -1264,6 +1279,11 @@ struct ADBC_EXPORT AdbcDriver {
struct AdbcPartitions*, int64_t*,
struct AdbcError*);
AdbcStatusCode (*MultiResultSetRelease)(struct AdbcMultiResultSet*, struct AdbcError*);

AdbcStatusCode (*ConnectionSetWarningHandler)(struct AdbcConnection*,
AdbcWarningHandler handler,
void* user_data, struct AdbcError*);

AdbcStatusCode (*StatementExecuteSchemaMulti)(struct AdbcStatement*,
struct AdbcMultiResultSet*,
struct AdbcError*);
Expand Down Expand Up @@ -1612,6 +1632,30 @@ ADBC_EXPORT
AdbcStatusCode AdbcConnectionRelease(struct AdbcConnection* connection,
struct AdbcError* error);

/// \brief Set a warning handler.
///
/// May be set before or after AdbcConnectionInit.
///
/// Drivers should not repeat warnings unnecessarily. For example, if a
/// warning is issued for a lossy conversion to Arrow data, ideally it would
/// be reported at most twice: once for the first occurrence, and/or a second
/// time at the end of the result set summarizing how many values were
/// affected.
///
/// \since ADBC API revision 1.2.0
Comment on lines +1637 to +1645
Copy link
Member

@paleolimbot paleolimbot Feb 22, 2026

Choose a reason for hiding this comment

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

This may be a future clarification, but as somebody setting a warning handler it would probably be good to have some expectation about how frequently this would be called, or perhaps some guidance as a driver author about how frequently it should be called. For example, if this is somehow used to warn about a lossy conversion from the internal database wire format to Arrow, driver authors should probably just warn about the first lossy value to be polite (this makes implementing the handler a bit easier).

It would probably be a good idea to explicitly state the thread safety of the handler (i.e., is the expectation that this handler can be called from any thread at any time, or is it the driver's responsibility to serialize warning calls?). It would be easier to implement a handler if it were the driver's responsibility to serialize calls.

Copy link
Contributor

@CurtHagenlocher CurtHagenlocher Feb 22, 2026

Choose a reason for hiding this comment

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

Agreed that we wouldn't want drivers to be too noisy. My ideal for something like a lossy conversion might be to warn about it the first time and then to report it at the end as a statistical summary e.g. "there were 3065 lossy conversions in this result".

I have a slight preference for this specific case to put serialization / concurrency safety onto the consumer but I don't feel strongly about it. The driver can't reasonably anticipate the host's concurrency requirements.

Copy link
Member

@paleolimbot paleolimbot Feb 22, 2026

Choose a reason for hiding this comment

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

My ideal for something like a lossy conversion might be to warn about it the first time and then to report it at the end as a statistical summary e.g. "there were 3065 lossy conversions in this result".

👍

I have a slight preference for this specific case to put serialization / concurrency safety onto the consumer

That makes sense. As a host application you might want to be defensive anyway. In R we have to call Rf_warning() from a very specific thread and so just serializing the calls won't help anyway.

Copy link
Member Author

@lidavidm lidavidm Feb 23, 2026

Choose a reason for hiding this comment

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

Thanks, I updated the wording a bit to clarify this.

/// \param[in] database The database.
/// \param[in] handler The warning handler to use; NULL removes the handler.
/// \param[in] user_data A user data pointer to be passed to the handler.
/// Must live at least until the connection is released or the warning
/// handler is replaced.
/// \param[out] error An optional location to return an error
/// message if necessary.
/// \return ADBC_STATUS_NOT_IMPLEMENTED if warning handlers are not supported
ADBC_EXPORT
AdbcStatusCode AdbcConnectionSetWarningHandler(struct AdbcConnection* connection,
AdbcWarningHandler handler,
void* user_data, struct AdbcError* error);

/// \brief Cancel the in-progress operation on a connection.
///
/// This can be called during AdbcConnectionGetObjects (or similar),
Expand Down
48 changes: 46 additions & 2 deletions go/adbc/drivermgr/arrow-adbc/adbc.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading