From: stephan Date: Fri, 28 Jul 2023 01:51:14 +0000 (+0000) Subject: More docs and cleanups related to the aggregate UDF state. Correct the OOM check... X-Git-Tag: version-3.43.0~47^2~143 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=75d3b1b5a20403cae8715e5d2500725565ca3789;p=thirdparty%2Fsqlite.git 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 --- diff --git a/ext/jni/src/c/sqlite3-jni.c b/ext/jni/src/c/sqlite3-jni.c index fba90108d1..6303d87553 100644 --- 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); diff --git a/ext/jni/src/org/sqlite/jni/SQLFunction.java b/ext/jni/src/org/sqlite/jni/SQLFunction.java index 7e7d817504..eaa83df9a9 100644 --- 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, and Window. */ 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 { private java.util.Map> map - = new java.util.HashMap>(); + = 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 xStep(sqlite3_context cx, T initialValue){ + public ValueHolder getAggregateState(sqlite3_context cx, T initialValue){ ValueHolder rc = map.get(cx.getAggregateContext()); if(null == rc){ map.put(cx.getAggregateContext(), rc = new ValueHolder(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 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 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 map = new ContextMap<>(); - /** - See ContextMap.xStep(). - */ - public final ValueHolder getAggregateState(sqlite3_context cx, T initialValue){ - return map.xStep(cx, initialValue); + //! See ContextMap.getAggregateState(). + protected final ValueHolder getAggregateState(sqlite3_context cx, T initialValue){ + return map.getAggregateState(cx, initialValue); } - /** - See ContextMap.xFinal(). - */ - public final T takeAggregateState(sqlite3_context cx){ - return map.xFinal(cx); + //! See ContextMap.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 inherits from Aggregate and each instance is + required to implemenat the inherited abstract methods from that + class. See Aggregate for information on managing the call + state across matching calls of the UDF callbacks. + */ public static abstract class Window extends Aggregate { - 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); } } diff --git a/ext/jni/src/org/sqlite/jni/sqlite3_context.java b/ext/jni/src/org/sqlite/jni/sqlite3_context.java index d6bc3012a1..bf2224dd5e 100644 --- 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 { /** 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; diff --git a/manifest b/manifest index 058fbe8d09..6237512571 100644 --- a/manifest +++ b/manifest @@ -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. diff --git a/manifest.uuid b/manifest.uuid index b7049ce4d7..28aa247cb8 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -6b56e4d62b4945e52978d00aa8e2984faa731c92a7e002e81524fcfcf8ba0cce \ No newline at end of file +ff53f1ccdc1780f2d9bd5f59804a76dbdf4f6b70696d3a7dbdbd96d1f8f6fa5c \ No newline at end of file