bug#61151: 30.0.50; sqlite-more-p is confusing

bug#61151: 30.0.50; sqlite-more-p is confusing

[Helmut Eller]

This code

  (let* ((db (sqlite-open))
         (stmt (sqlite-select db "values ('a',0), ('b',1)" nil 'set))
         (rows '()))
    (while (sqlite-more-p stmt)
      (push (sqlite-next stmt) rows))
    rows)

returns: (nil ("b" 1) ("a" 0))

I would expect: (("b" 1) ("a" 0))

I think it would be more natural, if sqlite-more-p would return false
before sqlite-next returns nil the first time.

Helmut



In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.16.0) of 2023-01-29 built on caladan
Repository revision: 254c75fc2935e7edef079166d90b231278115a2f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-xpm=ifavailable --with-jpeg=ifavailable
 --with-gif=ifavailable --with-tiff=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 GTK3
ZLIB

bug#61151: 30.0.50; sqlite-more-p is confusing

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Sun, 29 Jan 2023 15:52:34 +0100
> 
> This code
> 
>   (let* ((db (sqlite-open))
>          (stmt (sqlite-select db "values ('a',0), ('b',1)" nil 'set))
>          (rows '()))
>     (while (sqlite-more-p stmt)
>       (push (sqlite-next stmt) rows))
>     rows)
> 
> returns: (nil ("b" 1) ("a" 0))
> 
> I would expect: (("b" 1) ("a" 0))
> 
> I think it would be more natural, if sqlite-more-p would return false
> before sqlite-next returns nil the first time.

I'm not sure I understand why you need sqlite-more-p here.
sqlite-next returns nil when it hits the end, so it should be enough.
If we document this fact in the doc string of sqlite-next, would that
be good enough to satisfy your use cases?

bug#61151: 30.0.50; sqlite-more-p is confusing

[Helmut Eller]

On Sun, Jan 29 2023, Eli Zaretskii wrote:

> I'm not sure I understand why you need sqlite-more-p here.
> sqlite-next returns nil when it hits the end, so it should be enough.
> If we document this fact in the doc string of sqlite-next, would that
> be good enough to satisfy your use cases?

Yes, I guess it would be possible to get rid of sqlite-more-p.

However, sqlite-next seems to return nil once, but then it starts over.
E.g this

  (let* ((db (sqlite-open))
         (stmt (sqlite-select db "values ('a',0), ('b',1)" nil 'set))
         (rows '()))
    (list
     (sqlite-next stmt)
     (sqlite-next stmt)
     (sqlite-next stmt)
     (sqlite-next stmt)))
   
returns: (("a" 0) ("b" 1) nil ("a" 0))

This "wrap around" behavior is strange.

In summary, I think that sqlite-next should signal and error instead of
returning nil.  And sqlite-more-p should be used to detect the end of
the stream.  (Theoretically, sqlite-next could then also return zero
length rows, though that's probably not needed in practice.)

Helmut

bug#61151: 30.0.50; sqlite-more-p is confusing

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 61151@debbugs.gnu.org
> Date: Sun, 29 Jan 2023 17:05:07 +0100
> 
> On Sun, Jan 29 2023, Eli Zaretskii wrote:
> 
> > I'm not sure I understand why you need sqlite-more-p here.
> > sqlite-next returns nil when it hits the end, so it should be enough.
> > If we document this fact in the doc string of sqlite-next, would that
> > be good enough to satisfy your use cases?
> 
> Yes, I guess it would be possible to get rid of sqlite-more-p.
> 
> However, sqlite-next seems to return nil once, but then it starts over.

This should be easy to fix.

> In summary, I think that sqlite-next should signal and error instead of
> returning nil.

I think we can only signal an error if sqlite-next is called again
after it returned nil, otherwise it is not justified.  But I'm not an
expert on databases, so maybe I'm missing something.

> And sqlite-more-p should be used to detect the end of
> the stream.

I don't think it's possible without trying to read.

bug#61151: 30.0.50; sqlite-more-p is confusing

[Helmut Eller]

On Sun, Jan 29 2023, Eli Zaretskii wrote:

>> However, sqlite-next seems to return nil once, but then it starts over.
>
> This should be easy to fix.

The documentation[1] also says: 

  SQLITE_DONE means that the statement has finished executing
  successfully. sqlite3_step() should not be called again on this
  virtual machine without first calling sqlite3_reset() to reset the
  virtual machine back to its initial state.

So this would seems like prudent fix.

>> In summary, I think that sqlite-next should signal and error instead of
>> returning nil.
>
> I think we can only signal an error if sqlite-next is called again
> after it returned nil, otherwise it is not justified.  But I'm not an
> expert on databases, so maybe I'm missing something.
>
>> And sqlite-more-p should be used to detect the end of
>> the stream.
>
> I don't think it's possible without trying to read.

If we don't change the way the API works, then maybe the documentation
should mention that the proper way to iterate over the results is a bit
unusual. E.g.:

(let* ((db (sqlite-open))
       (stmt (sqlite-select db "values ('a',0), ('b',1)" nil 'set))
       (rows '())
       row)
  (while (progn (setq row (sqlite-next stmt))
		(sqlite-more-p stmt))
    (push row rows))
  rows)

And perhaps provide a function like sqlite-for-each that hides this
peculiarity.

Helmut

[1] https://sqlite.org/c3ref/step.html

bug#61151: 30.0.50; sqlite-more-p is confusing

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 61151@debbugs.gnu.org
> Date: Sun, 29 Jan 2023 19:20:30 +0100
> 
> On Sun, Jan 29 2023, Eli Zaretskii wrote:
> 
> >> However, sqlite-next seems to return nil once, but then it starts over.
> >
> > This should be easy to fix.
> 
> The documentation[1] also says: 
> 
>   SQLITE_DONE means that the statement has finished executing
>   successfully. sqlite3_step() should not be called again on this
>   virtual machine without first calling sqlite3_reset() to reset the
>   virtual machine back to its initial state.
> 
> So this would seems like prudent fix.

What is "this"?  Signaling an error after sqlite-next returns nil?  Or
something else?

> If we don't change the way the API works, then maybe the documentation
> should mention that the proper way to iterate over the results is a bit
> unusual. E.g.:
> 
> (let* ((db (sqlite-open))
>        (stmt (sqlite-select db "values ('a',0), ('b',1)" nil 'set))
>        (rows '())
>        row)
>   (while (progn (setq row (sqlite-next stmt))
> 		(sqlite-more-p stmt))
>     (push row rows))
>   rows)
> 
> And perhaps provide a function like sqlite-for-each that hides this
> peculiarity.

Let me think about this.

bug#61151: 30.0.50; sqlite-more-p is confusing

[Helmut Eller]

On Sun, Jan 29 2023, Eli Zaretskii wrote:

>> On Sun, Jan 29 2023, Eli Zaretskii wrote:
>> 
>> >> However, sqlite-next seems to return nil once, but then it starts over.
>> >
>> > This should be easy to fix.
>> 
>> The documentation[1] also says: 
>> 
>>   SQLITE_DONE means that the statement has finished executing
>>   successfully. sqlite3_step() should not be called again on this
>>   virtual machine without first calling sqlite3_reset() to reset the
>>   virtual machine back to its initial state.
>> 
>> So this would seems like prudent fix.
>
> What is "this"?  Signaling an error after sqlite-next returns nil?  Or
> something else?

I meant to not call sqlite3_step again after that it has returned
SQLITE_DONE.  Either by returning nil or signaling an error.

Helmut

bug#61151: 30.0.50; sqlite-more-p is confusing

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 61151@debbugs.gnu.org
> Date: Mon, 30 Jan 2023 07:57:09 +0100
> 
> On Sun, Jan 29 2023, Eli Zaretskii wrote:
> 
> >> On Sun, Jan 29 2023, Eli Zaretskii wrote:
> >> 
> >> >> However, sqlite-next seems to return nil once, but then it starts over.
> >> >
> >> > This should be easy to fix.
> >> 
> >> The documentation[1] also says: 
> >> 
> >>   SQLITE_DONE means that the statement has finished executing
> >>   successfully. sqlite3_step() should not be called again on this
> >>   virtual machine without first calling sqlite3_reset() to reset the
> >>   virtual machine back to its initial state.
> >> 
> >> So this would seems like prudent fix.
> >
> > What is "this"?  Signaling an error after sqlite-next returns nil?  Or
> > something else?
> 
> I meant to not call sqlite3_step again after that it has returned
> SQLITE_DONE.  Either by returning nil or signaling an error.

I went with the former.

I also documented that it returns nil when the statement finishes
execution.