SQL can't group-concat tags when using emacs-sqlite-builtin db connector

[tabroughton]

Description

When using the emacs-sqlite-builtin package the funcall to group-concat is not retruning multiple values when it should. This is not happening directly within the emacs-sqlite-builtin package but does happen via org-roam-db.

I believe this is an issue within the db handling of org-roam and is reproduced using org-roam-ui which uses (funcall group-concat tag (emacsql-escape-raw \, )) within an sql statement to create a list of tags for each node within the db. The result only returns a single tag for a node when using sqlite-builtin as the connector. When switching to emacs-sqlite it is fine. Note, as mentioned connecting to the db and running the same query directly using emacs-sqlite-builtin works, it is only when the query is passed into org-roam-db-query does it only return a single result when there should be more.

I have logged this issue also in org-roam-ui : org-roam/org-roam-ui#289

Steps to Reproduce

1. Emacs 29.1
2. create some nodes that have multiple filetags
3. ensure you are using package emacs-sqlite-builtin which will also depend on emacs-sqlite
4. set org-roam-database-connector to 'sqlite-builtin
5. syn the db
6. open the ui org-roam-ui-open
7. inspect any node that should have multiple tags, there will be only one
8. change the value of org-roam-database-connector to 'sqlite
9. open the ui org-roam-ui-open
10. all nodes that have multiple tags show multiple tags

Backtrace

There is no error to backtrace but here are the findings of my investigation:

The example SQL here is the following:

(org-roam-db-query [:select [id
                                file
                                title
                                level
                                pos
                                olp
                                properties
                                (funcall group-concat tag
                                         (emacsql-escape-raw \, ))]
                       :as tags
                       :from nodes
                       :left-join tags
                       :on (= id node_id)
                       :group :by id])

I have also tested the sql statement directly within certain parts of the system:

1. within org-roam-db-query does not return multiple tags when sqlite-builtin is used
2. within org-roam-db-query does return multiple tags when sqlite is used
3. within emacs-sqlite package directly connecting to the db, does return multiple tags
4. within emacs-sqlite-builtin package directly connecting to the same db, does return muliple tags

With reference to using emacs-sqlite-builtin directly this is the code I used to test it which returned multiple tags:

;appended to the bottom of `emacs-sqlite-builtin.el` to ensure that the builtin emacsql method is being used
(defvar db (emacsql-sqlite-builtin "~/.emacs.d/org-roam.db"))
(defun my-db-query (sql &rest args)
  (apply #'emacsql db sql args))			
(message "%S" (my-db-query [:select [
                                (funcall group-concat tag
                                         (emacsql-escape-raw \, ))]
                       :as tags
                       :from nodes
                       :left-join tags
                       :on (= id node_id)
                       :group :by id]))

Therefore I think it must be an issue in the way that org-roam-db is initialising/using the db/connector which for some odd reason means that funcall group-concat is not handled correctly... note: any automated tests may be passing since there are no errors thrown, but there are not multiple values concatenated when there should be.

Expected Results

muliple tags in the results of that sql query where nodes have multiple filetags

Actual Results

only a single tag is returned

Environment

• Emacs: GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)
• Framework: N/A
• Org: Org mode version 9.6.6 (release_9.6.6 @ /usr/share/emacs/29.1/lisp/org/)
• Org-roam: 2.2.2- sqlite-connector: sqlite-builtin

[wohanley]

I'm seeing the same issue, but I can't reproduce your success using emacs-sqlite-builtin directly. emacs -Q followed by:

(let ((default-directory  "~/.emacs.d/elpa/"))
  (normal-top-level-add-subdirs-to-load-path))

(require 'emacsql-sqlite-builtin)

(defvar db (emacsql-sqlite-builtin "~/.emacs.d/org-roam.db"))
(defun my-db-query (sql &rest args)
  (apply #'emacsql db sql args))
(message "%S" (my-db-query [:select [
                                (funcall group-concat tag
                                         (emacsql-escape-raw \, ))]
                       :as tags
                       :from nodes
                       :left-join tags
                       :on (= id node_id)
                       :group :by id]))

gives a result set that does not include multiple filetags, just the same as when I run the query through org-roam-db-query. I can't think of a reason we'd get different results - do you see the same thing if you try with emacs -Q?

org-roam-diagnostics below, and I'm loading emacsql-sqlite-builtin version 3.1.1.50-git from ~/.emacs.d/elpa/emacsql-20230417.1448, if that's interesting.

Environment

• Emacs: GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)
• Framework: Spacemacs
• Org: Org mode version 9.6.6 (release_9.6.6 @ /usr/share/emacs/29.1/lisp/org/)
• Org-roam: 2.2.2- sqlite-connector: sqlite

[technolapin]

I had this exact issue (I come from the org-roam post).

TLDR: the SQL querry happens normaly, but the post-processing lisp code is not meant for list values.

I have been testing things with a very simple case: I have one node containing 3 tags.

Basicaly, we would like emacs to do this:

sqlite> SELECT id, group_concat(tag) FROM nodes JOIN tags ON id = node_id GROUP BY id;
id                                      group_concat(tag)      
--------------------------------------  -----------------------
"3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f"  "Peter","Boss","Secret"

Let's look at "emacsql-send-message" in emacsql-sqlite-builtin.el :

(cl-defmethod emacsql-send-message
  ((connection emacsql-sqlite-builtin-connection) message)
  (condition-case err
      (mapcar (lambda (row)
                (mapcar (lambda (col)
                          (cond ((null col) nil)
                                ((equal col "") "")
                                ((numberp col) col)
                                (t (read col))))
                        row))
              (sqlite-select (oref connection handle) message nil nil))
    ((sqlite-error sqlite-locked-error)
     (if (stringp (cdr err))
         (signal 'emacsql-error (list (cdr err)))
       (pcase-let* ((`(,_ ,errstr ,errmsg ,errcode ,ext-errcode) err)
                    (`(,_ ,_ ,signal ,_)
                     (assq errcode emacsql-sqlite-error-codes)))
         (signal (or signal 'emacsql-error)
                 (list errmsg errcode ext-errcode errstr)))))
    (error
     (signal 'emacsql-error (cdr err)))))

I am not very used to look at lisp code so this big function is scary to me, but most of it is just error management and can be ignored in our case.

Basicaly, this can be reduced to

      (mapcar (lambda (row)
                (mapcar (lambda (col)
                          (cond ((null col) nil)
                                ((equal col "") "")
                                ((numberp col) col)
                                (t (read col))))
                        row))
              (sqlite-select (oref connection handle) message nil nil))

sqlite-select is called with the connection and the string of the sqlite query and returns the expected result

(sqlite-select (oref (org-roam-db) handle) "SELECT id, group_concat(tag, '\,') FROM nodes JOIN tags ON id = node_id GROUP BY id;" nil nil)

;; returns (("\"3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f\"" "\"Peter\"\\,\"Boss\"\\,\"Secret\""))

"\"Peter\"\\,\"Boss\"\\,\"Secret\"" is still a weird string, and we need to process it into a list (I guess?).
The mapcar is there for that, it read the strings of the various fields to turn them into actual values.

However, the values are filtered by the mapcar, and you would notice that there is no special case for list.
Therefore what happens is that the case t is applied on the list ("Peter" "Boss" "Secret"), which consist of calling read on it, effectively extracting the first element of the list.

(mapcar (lambda (row)
                (mapcar (lambda (col)
                          (cond ((null col) nil)
                                ((equal col "") "")
                                ((numberp col) col)
                                (t (read col))))
                        row))
              (sqlite-select (oref (org-roam-db) handle) "SELECT id, group_concat(tag, \"\\,\") FROM nodes JOIN tags ON id = node_id GROUP BY id;" nil nil))

;; returns (("3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f" "Peter"))

I have no idea of how to make this clean.

I changed the query to replace the separator ', ' by a simple whitespace and concatened parenthesis around the string to make everything a list.

(mapcar (lambda (row)
                (mapcar (lambda (col)
                          (let ((lst (cond ((null col) nil)
                                          ((equal col "") "")
                                          ((numberp col) col)
                                          (t (read (concat "(" col ")"))))))
                            
                               (cond ((null lst) nil)
                                     ((null (cdr lst)) (car lst))
                                     (t lst))))
                        row))
              (sqlite-select (oref (org-roam-db) handle) "SELECT id,  group_concat(tag, ' ') FROM nodes JOIN tags ON id = node_id GROUP BY id;" nil nil))

;; returns (("3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f" ("Peter" "Boss" "Secret")))

Maybe it would be preferable to keep using the commas, and maybe use regex, but that looks more complicated.

[wohanley]

@technolapin Thanks for this investigation! For the sake of comparison, it seems like the (working) approach in emacsql-sqlite relies on the base implementation of emacsql-parse in emacsql.el:

(cl-defmethod emacsql-parse ((connection emacsql-protocol-mixin))
  "Parse well-formed output into an s-expression."
  (with-current-buffer (emacsql-buffer connection)
    (goto-char (point-min))
    (let* ((standard-input (current-buffer))
           (value (read)))
      (if (eql value 'error)
          (emacsql-handle connection (read) (read))
        (prog1 value
          (unless (eq 'success (read))
            (emacsql-handle connection (read) (read))))))))

The important difference is that SQLite dumps its output into a buffer that we read all at once, i.e.:

(read "(\"3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f\" \"Peter\"\\,\"Boss\"\\,\"Secret\")")
  => ("3c24021f-b1f7-46c6-b6c6-dba5b4f2d18f" "Peter" \, "Boss" \, "Secret")

which works OK, although I'm still not really clear on how \, works.

With emacsql-sqlite-builtin, on the other hand, sqlite-select does part of the parsing for us, and hands us something that's already a list, which we then try to parse element by element, eventually resulting in a (read "\"Peter\"\\,\"Boss\"\\,\"Secret\"") => "Peter". n.b.: This is not reading a string that contains a list, it's reading a string that contains five separate forms.

I'm still not sure of the best way to fix this, but it's pretty clear what's broken, at least.