review comments

Author: tomas-villagesql

villagesql/sdk/include/villagesql/abi/types.h

@@ -282,7 +282,8 @@ typedef vef_keyring_result_t (*vef_write_keyring_fn)(const char *data_id,
 typedef struct {
   bool is_null;
   const char *str;  // null-terminated; valid only during the row callback
-  size_t str_len;
+  size_t str_len;   // number of bytes in str, not including the null terminator
+                    // (i.e. str_len == strlen(str)); 0 when is_null is true
 } vef_col_value_t;
 
 // Called once before the first row with column names and count.
@@ -316,8 +317,8 @@ typedef enum {
 //                that return no rows (INSERT, SET, etc.).
 //   ctx:         opaque pointer forwarded to meta_cb and row_cb.
 //   error_msg:   on VEF_QUERY_ERROR, a null-terminated description is written
-//                here; must point to a buffer of at least VEF_MAX_ERROR_LEN
-//                bytes; may be NULL if the caller does not need the message.
+//                here; if non-NULL, must point to a buffer of at least
+//                VEF_MAX_ERROR_LEN bytes.
 // Returns VEF_QUERY_OK, VEF_QUERY_ERROR, or VEF_QUERY_ABORTED.
 typedef vef_run_query_result_t (*vef_run_query_fn)(const char *sql,
                                                    size_t sql_len,

villagesql/sdk/include/villagesql/vsql/run_query.h

@@ -59,13 +59,16 @@ using RunQueryMetaCb =
     std::function<void(const std::vector<std::string_view> &col_names)>;
 
 // Row callback type: called once per result row with column values as strings.
-// SQL NULL is represented as an empty string_view (check is_null via the raw
-// ABI if you need to distinguish NULL from empty string).
+// SQL NULL is represented as a default-constructed string_view (data() ==
+// nullptr). An empty (non-NULL) string has data() != nullptr and size() == 0.
+// To distinguish NULL from empty string, check data() == nullptr.
 using RunQueryRowCb =
     std::function<void(const std::vector<std::string_view> &values)>;
 
 // Execute a SQL statement. Returns VEF_QUERY_OK, VEF_QUERY_ERROR, or
-// VEF_QUERY_ABORTED. On error, error_msg (if non-null) receives a description.
+// VEF_QUERY_ABORTED. On error, error_msg (if non-null, must point to a buffer
+// of at least VEF_MAX_ERROR_LEN bytes) receives a description.
+// inline: header-only implementation to avoid ODR violations across TUs.
 inline vef_run_query_result_t run_query(std::string_view sql,
                                         RunQueryMetaCb meta_cb,
                                         RunQueryRowCb row_cb,

villagesql/services/run_query.cc

@@ -182,8 +182,10 @@ static int cb_get_integer(void *ctx, longlong value) {
   return cb_get_longlong(ctx, value, 0);
 }
 
-// Decimal/date/time types: fall back to treating their value as a string.
-// In CS_TEXT_REPRESENTATION these typically come via get_string instead.
+// Decimal/date/time types: no-op stubs required by the callback interface.
+// With CS_TEXT_REPRESENTATION the server converts these to strings and routes
+// them through cb_get_string instead, so these callbacks are never called in
+// practice. They are present only to satisfy the st_command_service_cbs struct.
 static int cb_get_decimal(void * /*ctx*/, const decimal_t * /*value*/) {
   return 0;
 }
@@ -279,7 +281,12 @@ vef_run_query_result_t run_query(const char *sql, size_t sql_len,
     return VEF_QUERY_ERROR;
   }
 
-  // Propagate the caller's current database into the admin session.
+  // Propagate the caller's current database into the admin session so that
+  // unqualified table references in the SQL resolve correctly. This is
+  // intentional: extensions that run queries against tables in the caller's
+  // current database would otherwise need to fully-qualify every name.
+  // Extensions that want a clean session context should USE a specific database
+  // explicitly in their SQL (e.g. "USE mydb; SELECT ...").
   if (current_thd != nullptr) {
     LEX_CSTRING caller_db = current_thd->db();
     if (caller_db.str != nullptr && caller_db.length > 0) {

villagesql/test-extensions/vsql-run-query-test/src/extension.cc

@@ -78,6 +78,8 @@ void run_query_first_col(StringArg sql, StringResult out) {
       [&](const std::vector<std::string_view> &vals) {
         if (got_row) return;  // only keep the first row
         got_row = true;
+        // data() == nullptr means SQL NULL (empty string has data() !=
+        // nullptr).
         if (vals.empty() || vals[0].data() == nullptr) {
           first_is_null = true;
         } else {
@@ -103,7 +105,7 @@ void run_query_first_col(StringArg sql, StringResult out) {
 }
 
 VEF_GENERATE_ENTRY_POINTS(
-    make_extension("vsql_run_query_test", "0.0.1")
+    make_extension()
         .func(make_func<&run_query_count>("run_query_count")
                   .returns(INT)
                   .param(STRING)