Re: [PR] feat(format): add AdbcConnectionSetWarningHandler [arrow-adbc]

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

paleolimbot commented on code in PR #3872:
URL: https://github.com/apache/arrow-adbc/pull/3872#discussion_r2838223112


##########
c/include/arrow-adbc/adbc.h:
##########
@@ -1612,6 +1627,24 @@ ADBC_EXPORT
 AdbcStatusCode AdbcConnectionRelease(struct AdbcConnection* connection,
                                      struct AdbcError* error);
 
+/// \brief Set a warning handler.
+///
+/// May be set before or after AdbcConnectionInit.
+///
+/// \since ADBC API revision 1.2.0

Review Comment:
   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.



##########
c/include/arrow-adbc/adbc.h:
##########
@@ -1092,6 +1090,18 @@ AdbcStatusCode AdbcMultiResultSetNextPartitions(struct AdbcMultiResultSet* resul
                                                 struct AdbcError* error);
 /// @}
 
+/// \brief A warning handler function.
+///
+/// The handler must not block and must not call any other ADBC functions
+/// (besides releasing the warning).  The warning does not need to be released
+/// before returning.
+///
+/// \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);

Review Comment:
   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).



-- 
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