More docs and cleanups related to the aggregate UDF state. Correct the OOM check to behave properly if xFinal() is called without a matching xStep(), xValue(), or xInverse().

FossilOrigin-Name: ff53f1ccdc1780f2d9bd5f59804a76dbdf4f6b70696d3a7dbdbd96d1f8f6fa5c
2025-12-24 14:17:58 +03:00 · 2023-07-28 01:51:14 +00:00
parent 7d207bf483
commit 75d3b1b5a2
5 changed files with 115 additions and 101 deletions
--- a/ext/jni/src/c/sqlite3-jni.c
+++ b/ext/jni/src/c/sqlite3-jni.c
@@ -404,6 +404,33 @@ static void s3jni_free(void * p){
  if(p) sqlite3_free(p);
 }

+
+/*
+** This function is NOT part of the sqlite3 public API. It is strictly
+** for use by the sqlite project's own Java/JNI bindings.
+**
+** For purposes of certain hand-crafted JNI function bindings, we
+** need a way of reporting errors which is consistent with the rest of
+** the C API, as opposed to throwing JS exceptions. To that end, this
+** internal-use-only function is a thin proxy around
+** sqlite3ErrorWithMessage(). The intent is that it only be used from
+** JNI bindings such as sqlite3_prepare_v2/v3(), and definitely not
+** from client code.
+**
+** Returns err_code.
+*/
+static int s3jni_db_error(sqlite3*db, int err_code, const char *zMsg){
+  if( db!=0 ){
+    if( 0!=zMsg ){
+      const int nMsg = sqlite3Strlen30(zMsg);
+      sqlite3ErrorWithMsg(db, err_code, "%.*s", nMsg, zMsg);
+    }else{
+      sqlite3ErrorWithMsg(db, err_code, NULL);
+    }
+  }
+  return err_code;
+}
+
 /**
   Clears s's state, releasing any Java references. Before doing so,
   it calls s's xDestroy() method, ignoring the lack of that method or
@@ -724,13 +751,17 @@ static void * getNativePointer(JNIEnv * env, jobject pObj, const char *zClassNam

   isFinal must be 1 for xFinal() calls and 0 for all others.

-   Returns 0 on succes, SQLITE_NOMEM on allocation error.
+   Returns 0 on success. Returns SQLITE_NOMEM on allocation error,
+   noting that it will not allocate when isFinal is true. It returns
+   SQLITE_ERROR if there's a serious internal error in dealing with
+   the JNI state.
 */
-static int s3jni_setAggregateContext(JNIEnv * env, jobject jCx,
-                                sqlite3_context * pCx,
-                                int isFinal){
+static int udf_setAggregateContext(JNIEnv * env, jobject jCx,
+                                   sqlite3_context * pCx,
+                                   int isFinal){
  jmethodID setter;
  void * pAgg;
+  int rc = 0;
  struct NphCacheLine * const cacheLine =
    S3Global_nph_cache(env, ClassNames.sqlite3_context);
  if(cacheLine && cacheLine->klazz && cacheLine->midSetAgg){
@@ -746,43 +777,24 @@ static int s3jni_setAggregateContext(JNIEnv * env, jobject jCx,
      cacheLine->midSetAgg = setter;
    }
  }
-  pAgg = sqlite3_aggregate_context(pCx, isFinal ? 0 : 8);
-  if( pAgg ){
+  pAgg = sqlite3_aggregate_context(pCx, isFinal ? 0 : 4);
+  if( pAgg || isFinal ){
    (*env)->CallVoidMethod(env, jCx, setter, (jlong)pAgg);
-    IFTHREW_REPORT;
-  }
-  return pAgg ? (int)0 : SQLITE_NOMEM;
-}
-
-
-/*
-** This function is NOT part of the sqlite3 public API. It is strictly
-** for use by the sqlite project's own Java/JNI bindings.
-**
-** For purposes of certain hand-crafted JNI function bindings, we
-** need a way of reporting errors which is consistent with the rest of
-** the C API, as opposed to throwing JS exceptions. To that end, this
-** internal-use-only function is a thin proxy around
-** sqlite3ErrorWithMessage(). The intent is that it only be used from
-** JNI bindings such as sqlite3_prepare_v2/v3(), and definitely not
-** from client code.
-**
-** Returns err_code.
-*/
-static int s3jni_db_error(sqlite3*db, int err_code, const char *zMsg){
-  if( db!=0 ){
-    if( 0!=zMsg ){
-      const int nMsg = sqlite3Strlen30(zMsg);
-      sqlite3ErrorWithMsg(db, err_code, "%.*s", nMsg, zMsg);
-    }else{
-      sqlite3ErrorWithMsg(db, err_code, NULL);
+    IFTHREW {
+      EXCEPTION_REPORT;
+      EXCEPTION_CLEAR/*arguable, but so is propagation*/;
+      rc = s3jni_db_error(sqlite3_context_db_handle(pCx),
+                          SQLITE_ERROR,
+                          "sqlite3_context::setAggregateContext() "
+                          "unexpectedly threw.");
    }
+  }else{
+    assert(!pAgg);
+    rc = SQLITE_NOMEM;
  }
-  return err_code;
+  return rc;
 }

-
-
 /* Sets a native int32 value in OutputPointer.Int32 object ppOut. */
 static void setOutputInt32(JNIEnv * env, jobject ppOut, int v){
  jmethodID setter = 0;
@@ -1161,7 +1173,7 @@ static int udf_xFSI(sqlite3_context* pCx, int argc,
  if(rc) return rc;
  //MARKER(("UDF::%s.%s()\n", s->zFuncName, zFuncType));
  if( UDF_SCALAR != s->type ){
-    rc = s3jni_setAggregateContext(env, args.jcx, pCx, 0);
+    rc = udf_setAggregateContext(env, args.jcx, pCx, 0);
  }
  if( 0 == rc ){
    (*env)->CallVoidMethod(env, s->jObj, xMethodID, args.jcx, args.jargv);
@@ -1187,7 +1199,7 @@ static int udf_xFV(sqlite3_context* cx, UDFState * s,
  }
  //MARKER(("UDF::%s.%s()\n", s->zFuncName, zFuncType));
  if( UDF_SCALAR != s->type ){
-    rc = s3jni_setAggregateContext(env, jcx, cx, 1);
+    rc = udf_setAggregateContext(env, jcx, cx, 1);
  }
  if( 0 == rc ){
    (*env)->CallVoidMethod(env, s->jObj, xMethodID, jcx);
--- a/ext/jni/src/org/sqlite/jni/SQLFunction.java
+++ b/ext/jni/src/org/sqlite/jni/SQLFunction.java
@@ -18,42 +18,41 @@ package org.sqlite.jni;
   sqlite3_create_function() JNI-bound API to give that native code
   access to the callback functions needed in order to implement SQL
   functions in Java. This class is not used by itself: see the
-   three inner classes.
-
-   Note that if a given function is called multiple times in a single
-   SQL statement, e.g. SELECT MYFUNC(A), MYFUNC(B)..., then the
-   context object passed to each one will be different. This is most
-   significant for aggregates and window functions, since they must
-   assign their results to the proper context.
-
-   TODO: add helper APIs to map sqlite3_context instances to
-   func-specific state and to clear that when the aggregate or window
-   function is done.
+   inner classes Scalar, Aggregate<T>, and Window<T>.
 */
 public abstract class SQLFunction {

  /**
     ContextMap is a helper for use with aggregate and window
     functions, to help them manage their accumulator state across
-     calls to xStep() and xFinal(). It works by mapping
+     calls to the UDF's callbacks.
+
+     If a given aggregate or window function is called multiple times
+     in a single SQL statement, e.g. SELECT MYFUNC(A), MYFUNC(B)...,
+     then the clients need some way of knowing which call is which so
+     that they can map their state between their various UDF callbacks
+     and reset it (if needed) via xFinal(). This class takes care of
+     such mappings.
+
+     This class works by mapping
     sqlite3_context::getAggregateContext() to a single piece of state
-     which persists across a set of 0 or more SQLFunction.xStep()
-     calls and 1 SQLFunction.xFinal() call.
-   */
+     which persists across a "matching set" of the UDF's callbacks.
+  */
  public static final class ContextMap<T> {
    private java.util.Map<Long,ValueHolder<T>> map
-      = new java.util.HashMap<Long,ValueHolder<T>>();
+      = new java.util.HashMap<>();

    /**
-       Should be called from a UDF's xStep() method, passing it that
-       method's first argument and an initial value for the persistent
-       state. If there is currently no mapping for
-       cx.getAggregateContext() within the map, one is created, else
-       an existing one is preferred.  It returns a ValueHolder which
-       can be used to modify that state directly without having to put
-       a new result back in the underlying map.
+       Should be called from a UDF's xStep(), xValue(), and xInverse()
+       methods, passing it that method's first argument and an initial
+       value for the persistent state. If there is currently no
+       mapping for cx.getAggregateContext() within the map, one is
+       created using the given initial value, else an existing one is
+       use and the 2nd argument is ignored.  It returns a ValueHolder
+       which can be used to modify that state directly without
+       requiring that the user update the underlying map.
    */
-    public ValueHolder<T> xStep(sqlite3_context cx, T initialValue){
+    public ValueHolder<T> getAggregateState(sqlite3_context cx, T initialValue){
      ValueHolder<T> rc = map.get(cx.getAggregateContext());
      if(null == rc){
        map.put(cx.getAggregateContext(), rc = new ValueHolder<T>(initialValue));
@@ -63,13 +62,13 @@ public abstract class SQLFunction {

    /**
       Should be called from a UDF's xFinal() method and passed that
-       method's first argument. This function returns the value
-       associated with cx.getAggregateContext(), or null if
-       this.xStep() has not been called to set up such a mapping. That
-       will be the case if an aggregate is used in a statement which
-       has no result rows.
+       method's first argument. This function removes the value
+       associated with cx.getAggregateContext() from the map and
+       returns it, returning null if no other UDF method has not been
+       called to set up such a mapping. That will be the case if an
+       aggregate is used in a statement which has no result rows.
    */
-    public T xFinal(sqlite3_context cx){
+    public T takeAggregateState(sqlite3_context cx){
      final ValueHolder<T> h = map.remove(cx.getAggregateContext());
      return null==h ? null : h.value;
    }
@@ -79,43 +78,47 @@ public abstract class SQLFunction {
  public static abstract class Scalar extends SQLFunction {
    public abstract void xFunc(sqlite3_context cx, sqlite3_value[] args);
    /**
-       Optionally override to be notified when the function is
-       finalized by SQLite.
+       Optionally override to be notified when the UDF is finalized by
+       SQLite.
    */
    public void xDestroy() {}
  }

-  //! Subclass for creating aggregate functions.
+  /**
+     SQLFunction Subclass for creating aggregate functions.  Its T is
+     the data type of its "accumulator" state, an instance of which is
+     intended to be be managed using the getAggregateState() and
+     takeAggregateState() methods.
+  */
  public static abstract class Aggregate<T> extends SQLFunction {
    public abstract void xStep(sqlite3_context cx, sqlite3_value[] args);
    public abstract void xFinal(sqlite3_context cx);
+
+    //! See Scalar.xDestroy()
    public void xDestroy() {}

    private final ContextMap<T> map = new ContextMap<>();

-    /**
-       See ContextMap<T>.xStep().
-    */
-    public final ValueHolder<T> getAggregateState(sqlite3_context cx, T initialValue){
-      return map.xStep(cx, initialValue);
+    //! See ContextMap<T>.getAggregateState().
+    protected final ValueHolder<T> getAggregateState(sqlite3_context cx, T initialValue){
+      return map.getAggregateState(cx, initialValue);
    }

-    /**
-       See ContextMap<T>.xFinal().
-    */
-    public final T takeAggregateState(sqlite3_context cx){
-      return map.xFinal(cx);
+    //! See ContextMap<T>.takeAggregateState().
+    protected final T takeAggregateState(sqlite3_context cx){
+      return map.takeAggregateState(cx);
    }
  }

-  //! Subclass for creating window functions.
+  /**
+     An SQLFunction subclass for creating window functions.  Note that
+     Window<T> inherits from Aggregate<T> and each instance is
+     required to implemenat the inherited abstract methods from that
+     class. See Aggregate<T> for information on managing the call
+     state across matching calls of the UDF callbacks.
+  */
  public static abstract class Window<T> extends Aggregate<T> {
-    public Window(){
-      super();
-    }
-    //public abstract void xStep(sqlite3_context cx, sqlite3_value[] args);
    public abstract void xInverse(sqlite3_context cx, sqlite3_value[] args);
-    //public abstract void xFinal(sqlite3_context cx);
    public abstract void xValue(sqlite3_context cx);
  }
 }
--- a/ext/jni/src/org/sqlite/jni/sqlite3_context.java
+++ b/ext/jni/src/org/sqlite/jni/sqlite3_context.java
@@ -34,11 +34,10 @@ public class sqlite3_context extends NativePointerHolder<sqlite3_context> {
  /**
     If this object is being used in the context of an aggregate or
     window UDF, the UDF binding layer will set a unique context value
-     here. That value will be the same across matching calls to the
-     xStep() and xFinal() routines, as well as xValue() and xInverse()
-     in window UDFs. This value can be used as a key to map state
-     which needs to persist across such calls, noting that such state
-     should be cleaned up via xFinal().
+     here, else this will return 0. That value will be the same across
+     matching calls to the UDF callbacks. This value can be used as a
+     key to map state which needs to persist across such calls, noting
+     that such state should be cleaned up via xFinal().
  */
  public long getAggregateContext(){
    return aggcx;
--- a/16
+++ b/16
@@ -1,5 +1,5 @@
-C Add\san\sOOM\scheck\sto\sthe\sprevious\scheck-in.\sMinor\sinternal\sAPI\srenaming.
-D 2023-07-28T01:19:44.606
+C More\sdocs\sand\scleanups\srelated\sto\sthe\saggregate\sUDF\sstate.\sCorrect\sthe\sOOM\scheck\sto\sbehave\sproperly\sif\sxFinal()\sis\scalled\swithout\sa\smatching\sxStep(),\sxValue(),\sor\sxInverse().
+D 2023-07-28T01:51:14.668
 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1
 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea
 F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724
@@ -232,20 +232,20 @@ F ext/icu/icu.c c074519b46baa484bb5396c7e01e051034da8884bad1a1cb7f09bbe6be3f0282
 F ext/icu/sqliteicu.h fa373836ed5a1ee7478bdf8a1650689294e41d0c89c1daab26e9ae78a32075a8
 F ext/jni/GNUmakefile 56a014dbff9516774d895ec1ae9df0ed442765b556f79a0fc0b5bc438217200d
 F ext/jni/README.md 042762dbf047667783a5bd0aec303535140f302debfbd259c612edf856661623
-F ext/jni/src/c/sqlite3-jni.c 7e76652684c38df7c48ac3300056601202b0c45d91c5f3671725e17c3c69ec6a
+F ext/jni/src/c/sqlite3-jni.c 9464d7f186c52cecd4c6ac91d3da35f29fd98923a048befc8d2d872edd639a41
 F ext/jni/src/c/sqlite3-jni.h c9bb150a38dce09cc2794d5aac8fa097288d9946fbb15250fd0a23c31957f506
 F ext/jni/src/org/sqlite/jni/BusyHandler.java 1b1d3e5c86cd796a0580c81b6af6550ad943baa25e47ada0dcca3aff3ebe978c
 F ext/jni/src/org/sqlite/jni/Collation.java 8dffbb00938007ad0967b2ab424d3c908413af1bbd3d212b9c9899910f1218d1
 F ext/jni/src/org/sqlite/jni/NativePointerHolder.java 70dc7bc41f80352ff3d4331e2e24f45fcd23353b3641e2f68a81bd8262215861
 F ext/jni/src/org/sqlite/jni/OutputPointer.java 08a752b58a33696c5eaf0eb9361a0966b188dec40f4a3613eb133123951f6c5f
 F ext/jni/src/org/sqlite/jni/ProgressHandler.java 5a1d7b2607eb2ef596fcf4492a49d1b3a5bdea3af9918e11716831ffd2f02284
-F ext/jni/src/org/sqlite/jni/SQLFunction.java d77e0a4bb6bc0d65339aeacd6b20fc7e3b8a05f899c1f0ead90dda61f0a01522
+F ext/jni/src/org/sqlite/jni/SQLFunction.java b176c46828a52084dd3a39e5084d0b0ce12dcaf2abe719a58f4d1d92733e1136
 F ext/jni/src/org/sqlite/jni/SQLite3Jni.java 3582b30c0fb1cb39e25b9069fe8c9e2fe4f2659f4d38437b610e46143e163610
 F ext/jni/src/org/sqlite/jni/Tester1.java 2334d1dd0efc22179654c586065c77d904830d736059b4049f9cd9e6832565bd
 F ext/jni/src/org/sqlite/jni/Tracer.java c2fe1eba4a76581b93b375a7b95ab1919e5ae60accfb06d6beb067b033e9bae1
 F ext/jni/src/org/sqlite/jni/ValueHolder.java f022873abaabf64f3dd71ab0d6037c6e71cece3b8819fa10bf26a5461dc973ee
 F ext/jni/src/org/sqlite/jni/sqlite3.java c7d0500c7269882243aafb41425928d094b2fcbdbc2fd1caffc276871cd3fae3
-F ext/jni/src/org/sqlite/jni/sqlite3_context.java 4a0b22226705a4f89d9c8093e0f51a8991cc0464864120970c915695afbba4e2
+F ext/jni/src/org/sqlite/jni/sqlite3_context.java 4e7eebc8a5c85ecfbae3aa2c4ddb7f1ca861c218d3829d31afe16f6b11104213
 F ext/jni/src/org/sqlite/jni/sqlite3_stmt.java 3193693440071998a66870544d1d2314f144bea397ce4c3f83ff225d587067a0
 F ext/jni/src/org/sqlite/jni/sqlite3_value.java f9d8c0766b1d1b290564cb35db8d37be54c42adc8df22ee77b8d39e3e93398cd
 F ext/lsm1/Makefile a553b728bba6c11201b795188c5708915cc4290f02b7df6ba7e8c4c943fd5cd9
@@ -2067,8 +2067,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93
 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc
 F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e
 F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0
-P 640574984741c7a9472d7f8be7bce87e736d7947ce673ae4a25008d74238ad90
-R ce71c2fd0629446f06e751d7b64d5f7d
+P 6b56e4d62b4945e52978d00aa8e2984faa731c92a7e002e81524fcfcf8ba0cce
+R 0fe909577d9a504bc5127b45fc11fe45
 U stephan
-Z 3429cf1394e9a8e9214fcef9821d7359
+Z 8f663d2013372069850b9c169f30fdc3
 # Remove this line to create a well-formed Fossil manifest.
--- a/manifest.uuid
+++ b/manifest.uuid
@@ -1 +1 @@
-6b56e4d62b4945e52978d00aa8e2984faa731c92a7e002e81524fcfcf8ba0cce
+ff53f1ccdc1780f2d9bd5f59804a76dbdf4f6b70696d3a7dbdbd96d1f8f6fa5c