[tarantool-patches] Re: [PATCH v1 05/12] sql: put analyze helpers to FuncDef cache

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Tue, 9 Jul 2019 18:56:34 +0300

Prior patches are OK and I guess can be pushed -
they are quite trivial (except for previous one - which
introduces persistent functions in Tarantool). Hence you
can swap order of patches (i.e. this and previous one) so
that push series of patches independently from non-trivial.
Also, please fix compilation errors which appear on Travis.

On 8 Jul 2019, at 14:26, Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx> 
wrote:

Previously analyze functions refer to statically defined
service FuncDef context. We need to change this approach due we
going to rework the builtins functions machinery in following
patches.

Needed for #4113, #2200, #2233


Unfortunately, analyze is currently disabled, so there’s
no opportunity to check functionality of this patch :)

@@ -986,8 +956,11 @@ vdbe_emit_analyze_space(struct Parse *parse, struct 
space *space)
              sqlVdbeAddOp3(v, OP_MakeRecord, stat_key_reg,
                                pk_part_count, key_reg);
              assert(chng_reg == (stat4_reg + 1));
+             struct FuncDef *push_func =
+                     sqlFindFunction(sql_get(), "_sql_stat_push", 3, 0);
+             assert(push_func != NULL);
              sqlVdbeAddOp4(v, OP_Function0, 1, stat4_reg, tmp_reg,
-                               (char *)&statPushFuncdef, P4_FUNCDEF);
+                           (char *)push_func, P4_FUNCDEF);
              sqlVdbeChangeP5(v, 3);
              sqlVdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
              /* Add the entry to the stat1 table. */
@@ -1774,3 +1747,58 @@ fail:
      box_txn_rollback();
      return -1;
}
+
+/**
+ * The implementation of the sql_record() function. This function
+ * accepts a single argument of any type. The return value is a
+ * formatted database record (a blob) containing the argument
+ * value.
+ *
+ * This is used to convert the value stored in the 'sample'
+ * column of the sql_stat4 table to the record format sql uses
+ * internally.
+ */
+static void
+sql_record_func(sql_context * context, int argc, sql_value ** argv)

I guess there’s no need in this function at all.
Could you simply remove it?

+{
+     const int file_format = 1;
+     u32 iSerial;            /* Serial type */
+     int nSerial;            /* Bytes of space for iSerial as varint */
+     u32 nVal;               /* Bytes of space required for argv[0] */
+     int nRet;
+     sql *db;
+     u8 *aRet;
+
+     UNUSED_PARAMETER(argc);
+     iSerial = sqlVdbeSerialType(argv[0], file_format, &nVal);
+     nSerial = sqlVarintLen(iSerial);
+     db = sql_context_db_handle(context);
+
+     nRet = 1 + nSerial + nVal;
+     aRet = sqlDbMallocRawNN(db, nRet);
+     if (aRet == 0) {
+             context->is_aborted = true;
+     } else {
+             aRet[0] = nSerial + 1;
+             putVarint32(&aRet[1], iSerial);
+             sqlVdbeSerialPut(&aRet[1 + nSerial], argv[0], iSerial);
+             sql_result_blob(context, aRet, nRet, SQL_TRANSIENT);
+             sqlDbFree(db, aRet);
+     }
+}
+
+#define FUNCTION_ANALYZE(zName, nArg, xFunc) \
+  {nArg, SQL_FUNC_CONSTANT, NULL, 0, xFunc, 0, #zName, {0},\
+   FIELD_TYPE_ANY, false}

Could you move this macro to other FUNCTION_… ones (sqlInt.h)?

+
+void
+sql_register_analyze_builtins(void)
+{
+     static FuncDef aAnalyzeTableFuncs[] = {

Please, use names according to our code style.


Other related posts: