feat(format): add AdbcConnectionSetWarningHandler#3872
feat(format): add AdbcConnectionSetWarningHandler#3872lidavidm merged 2 commits intoapache:spec-1.2.0from
Conversation
|
CC @CurtHagenlocher @zeroshade |
lidavidm
changed the title
lidavidm
requested review from
CurtHagenlocher,
amoeba,
paleolimbot and
zeroshade
| /// \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); |
There was a problem hiding this 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).
| /// May be set before or after AdbcConnectionInit. | ||
| /// | ||
| /// \since ADBC API revision 1.2.0 |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, I updated the wording a bit to clarify this.
3 participants
Closes #1243.