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.
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
feat: duckdb_api and custom_user_agent options #9603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't seem to be using these new values anywhere yet, is that intentional?
tools/jdbc/src/jni/duckdb_java.cpp
Outdated
@@ -310,13 +312,22 @@ static const char *const JDBC_STREAM_RESULTS = "jdbc_stream_results"; | |||
jobject _duckdb_jdbc_startup(JNIEnv *env, jclass, jbyteArray database_j, jboolean read_only, jobject props) { | |||
auto database = byte_array_to_string(env, database_j); | |||
DBConfig config; | |||
config.options.duckdb_api = "jni"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call this Java? You also seem to be overriding it in the Java layer, which is a bit confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll rename to "java". This override is here just in case someone uses Java without JDBC -- making the distinction between "java" and "jdbc" would make this obscure case easier to troubleshoot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if that would be possible? The Java API is the the JDBC API
public static void test_user_agent() throws Exception { | ||
try (Connection conn = DriverManager.getConnection("jdbc:duckdb:")) { | ||
try (PreparedStatement stmt1 = | ||
conn.prepareStatement("SELECT value FROM duckdb_settings() WHERE name = 'custom_user_agent'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: use the current_setting scalar function
Can be potentially added later, but maybe it's worth raising together with this: could it make sense to have a possibility to set this via a C++ define? Idea would be that then also CLI could get relevant tags by doing something like: This would allow also to guarantee the invariant that a custom_user_agent is always non-empty (either as the default value for USER_AGENT, say Idea (if this makes sense) is being able to distinguish whether something has been built (and likely distributed) via our github releases, via homebrew or via other means. That could be useful for example to give instructions to users on how to update duckdb itself (like if duckdb is distributed via homebrew, first the version can be checked vs latest release, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! LGTM - some minor comments:
@maiadegraaf could you perhaps look at the ODBC portion of the PR?
src/main/capi/duckdb-c.cpp
Outdated
} | ||
|
||
if (db_config->options.duckdb_api.empty()) { | ||
db_config->options.duckdb_api = "capi"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing the check here perhaps it's cleaner/easier to set duckdb_api
to "capi"
in duckdb_create_config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some logic for setting capi
is still needed here because duckdb_open_ext()
is often called with nullptr
passed in for config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ODBC stuff looks good to me. Just a heads up, I'm currently reworking the connection string so the changes in tools/odbc/driver.cpp
will most likely be altered shortly.
The tests look good too, can they be moved to tests/connect.cpp
instead, since they test the connection string rather than SQLSetEnvAttr
or SQLGetEnvAttr
.
@carlopi I love the idea of knowing how duckdb got built; it would be very helpful for troubleshooting. Maybe a separate compile-time constant DUCKDB_DISTRIBUTION, similar to DUCKDB_VERSION, that could then become a component returned from |
@Mause I was thinking the typical usage would be invocation through the user-facing |
Why not use it as the |
Thanks!
That's a good idea, but let's add that in a later PR. |
Introduces two new settings:
duckdb_api
representing the DuckDB surface (C API, Python, JDBC etc.) andcustom_user_agent
that is settable by the user. Additionally, a table functionpragma_user_agent()
andPRAGMA user_agent
can be used to query the overall user-agent value for the current session.Example usage from CLI:
Examples of overall user-agent strings from different APIs, assuming that the end user passed
CUSTOM_STRING
as the value ofcustom_user_agent
:duckdb/v0.9.2-dev256(linux_amd64) capi CUSTOM_STRING
duckdb/v0.9.2-dev256(linux_amd64) python CUSTOM_STRING
test_custom_user_agent duckdb/v0.9.2-dev256(linux_amd64) jdbc CUSTOM_STRING
duckdb/v0.9.2-dev253(linux_amd64) odbc CUSTOM_STRING
cc: @bleskes