Bug 6770 - SQL function not translated correctly
Summary: SQL function not translated correctly
Status: RESOLVED INVALID
Alias: None
Product: SQL
Classification: Unclassified
Component: all (show other bugs)
Version: 11.33.11 (Apr2019-SP1)
Hardware: Other Linux
: Normal normal
Assignee: SQL devs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-07 14:00 CEST by Roberto Cornacchia
Modified: 2019-10-09 20:41 CEST (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Roberto Cornacchia 2019-10-07 14:00:19 CEST
User-Agent:       Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36
Build Identifier: 

START TRANSACTION;

CREATE OR REPLACE FUNCTION mycontains(s STRING, p STRING)
RETURNS BOOLEAN
BEGIN
	RETURN SELECT s like '%'||p||'%';
END;


CREATE TABLE t(s STRING);
INSERT INTO t VALUES ('apple'),('pear'),('banana'),('orange');

sql>SELECT * FROM t WHERE s like '%an%';
+--------+
| s      |
+========+
| banana |
| orange |
+--------+
2 tuples

sql>SELECT * FROM t WHERE mycontains(s,'an');
+---+
| s |
+===+
+---+
0 tuples


The explain shows indeed an empty loop over the bat:

sql>explain SELECT * FROM t WHERE mycontains(s,'an');
+----------------------------------------------------------------------------------------------------------------------+
| mal                                                                                                                  |
+======================================================================================================================+
| function user.s62_1():void;                                                                                          |
|     X_2:void := querylog.define("select * from t where mycontains(s,\\'an\\');":str, "sequential_pipe":str, 23:int); |
|     X_23:bat[:str] := bat.new(nil:str);                                                                              |
|     X_30:bat[:str] := bat.append(X_23:bat[:str], "spinque.t":str);                                                   |
|     X_25:bat[:str] := bat.new(nil:str);                                                                              |
|     X_32:bat[:str] := bat.append(X_25:bat[:str], "s":str);                                                           |
|     X_26:bat[:str] := bat.new(nil:str);                                                                              |
|     X_33:bat[:str] := bat.append(X_26:bat[:str], "clob":str);                                                        |
|     X_27:bat[:int] := bat.new(nil:int);                                                                              |
|     X_35:bat[:int] := bat.append(X_27:bat[:int], 0:int);                                                             |
|     X_29:bat[:int] := bat.new(nil:int);                                                                              |
|     X_36:bat[:int] := bat.append(X_29:bat[:int], 0:int);                                                             |
|     X_5:int := sql.mvc();                                                                                            |
|     C_6:bat[:oid] := sql.tid(X_5:int, "spinque":str, "t":str);                                                       |
|     X_9:bat[:str] := sql.bind(X_5:int, "spinque":str, "t":str, "s":str, 0:int);                                      |
|     X_12:bat[:str] := algebra.projection(C_6:bat[:oid], X_9:bat[:str]);                                              |
|     X_83:bat[:bit] := bat.new(nil:bit);                                                                              |
| barrier (X_86:oid, X_87:str) := iterator.new(X_12:bat[:str]);                                                        |
|     redo (X_86:oid, X_87:str) := iterator.next(X_12:bat[:str]);                                                      |
| exit (X_86:oid, X_87:str);                                                                                           |
|     X_14:bat[:bit] := X_83:bat[:bit];                                                                                |
|     C_19:bat[:oid] := algebra.thetaselect(X_14:bat[:bit], true:bit, "==":str);                                       |
|     X_21:bat[:str] := algebra.projection(C_19:bat[:oid], X_12:bat[:str]);                                            |
|     sql.resultSet(X_30:bat[:str], X_32:bat[:str], X_33:bat[:str], X_35:bat[:int], X_36:bat[:int], X_21:bat[:str]);   |
| end user.s62_1;                                                                                                      |


Reproducible: Always
Comment 1 Sjoerd Mullender cwiconfidential 2019-10-07 14:49:24 CEST
I cannot reproduce the error:

sql>SELECT * FROM t WHERE mycontains(s,'an');
+--------+
| s      |
+========+
| banana |
| orange |
+--------+
2 tuples

Both in the Apr2019-SP1 release and on the branch head.
Comment 2 Pedro Ferreira 2019-10-07 14:57:08 CEST
By forcing mitosis on Apr2019 branch it triggers the error:
GDK reported error: BATproject: does not match always
Comment 3 Pedro Ferreira 2019-10-07 15:02:44 CEST
However on Nov2019 branch it always runs fine.
Comment 4 Roberto Cornacchia 2019-10-07 15:34:15 CEST
My bad. 
It works fine if I disable all our own changes.
Comment 5 Sjoerd Mullender cwiconfidential 2019-10-07 15:38:48 CEST
Then we can close this.
Comment 6 Roberto Cornacchia 2019-10-07 16:07:16 CEST
For completeness, the issue was due indeed from one of my own changes. 
However, it is triggered in this specific case because a not fully consistent translation to MAL.

This is the loop with optimizer set to minimal_pipe():

| barrier (X_52:oid, X_53:str) := iterator.new(X_12:bat[:str]);
|     X_55:bit := user.mycontains(X_53:str, X_13:str);
|     bat.append(X_49:bat[:bit], X_55:bit);
|     redo (X_52:oid, X_53:str) := iterator.next(X_12:bat[:str]);
| exit (X_52:oid, X_53:str);

The body later disappears in my case because my change makes bat.append() a side-effect-free operation. Thus, not being assigned to any variable, it gets removed by a later optimizer.

Not assigning the result of bat.append() to a variable isn't good practice, I believe. It does define one, and ignoring it because of the side-effect assumption isn't clean.
Indeed, it is always assigned to a variable in the rest of the MAL plan. This part is not translated consistently with the rest.
Comment 7 Roberto Cornacchia 2019-10-07 16:46:54 CEST
May I suggest something like this?

diff -r c83498edd5c4 monetdb5/optimizer/opt_multiplex.c
--- a/monetdb5/optimizer/opt_multiplex.c        Fri Oct 04 14:01:38 2019 +0200
+++ b/monetdb5/optimizer/opt_multiplex.c        Mon Oct 07 16:45:44 2019 +0200
@@ -157,7 +157,8 @@
        for (i = 0; i < pci->retc; i++) {
                InstrPtr a = newFcnCall(mb, batRef, appendRef);
                a = pushArgument(mb, a, resB[i]);
-               (void) pushArgument(mb, a, getArg(q,i));
+               a = pushArgument(mb, a, getArg(q,i));
+               getArg(a,0) = resB[i];
        }
 
 /* redo (h,r):= iterator.next(refBat); */
Comment 8 Roberto Cornacchia 2019-10-07 16:51:16 CEST
With that patch, the output looks fine:


|     X_49:bat[:bit] := bat.new(nil:bit);
| barrier (X_52:oid, X_53:str) := iterator.new(X_12:bat[:str]);
|     X_55:bit := user.mycontains(X_53:str, X_13:str);
|     X_49:bat[:bit] := bat.append(X_49:bat[:bit], X_55:bit);
|     redo (X_52:oid, X_53:str) := iterator.next(X_12:bat[:str]);
| exit (X_52:oid, X_53:str);
|     X_14:bat[:bit] := X_49:bat[:bit];
Comment 9 MonetDB Mercurial Repository cwiconfidential 2019-10-09 20:41:09 CEST
Changeset a757e8d68362, made by Martin Kersten <mk@cwi.nl> in the MonetDB repo, refers to this bug.

For complete details, see https://dev.monetdb.org/hg/MonetDB?cmd=changeset;node=a757e8d68362

Changeset description:

	Applying the patch from bug 6770, reported by Cornacchio