Hi, Alexander, Looks good, thanks. Great commit comment too. Just a few questions and test suggestions - see below. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org On Aug 23, Alexander Barkov wrote:
revision-id: c46e7c5cb64 (mariadb-11.4.1-5-gc46e7c5cb64) parent(s): ae6684d79f4 author: Alexander Barkov committer: Alexander Barkov timestamp: 2024-02-20 13:49:54 +0400 message:
MDEV-12252 ROW data type for stored function return values
diff --git a/mysql-test/main/sp-anchor-row-type-table.test b/mysql-test/main/sp-anchor-row-type-table.test index 7123f9160db..2fbcfbffc1b 100644 --- a/mysql-test/main/sp-anchor-row-type-table.test +++ b/mysql-test/main/sp-anchor-row-type-table.test @@ -939,3 +939,70 @@ DROP TABLE t1; --echo # --echo # End of 10.4 tests --echo # + +--echo # +--echo # Start of 11.3 tests +--echo # + +--echo # +--echo # MDEV-12252 ROW data type for stored function return values +--echo # + +--error ER_PARSE_ERROR +CREATE FUNCTION f1() RETURNS ROW TYPE OF step1.step2.step3 RETURN ROW(1,2); + +CREATE FUNCTION f1() RETURNS ROW TYPE OF t1 RETURN ROW(1,2);
you don't resolve t1 at CREATE FUNCTION time. why? is it how TYPE OF works in DECLARE too?
+SHOW CREATE FUNCTION f1; +--error ER_NO_SUCH_TABLE +SELECT f1(); +DROP FUNCTION f1; + +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN ROW(1,2); +SHOW CREATE FUNCTION f1; +--error ER_NO_SUCH_TABLE +SELECT f1(); +DROP FUNCTION f1; + + +CREATE TABLE t1 (a INT, b VARCHAR(32)); +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN (SELECT * FROM t1); +SHOW CREATE FUNCTION f1; +DELIMITER $$; +CREATE PROCEDURE p1() +BEGIN + DECLARE r ROW TYPE OF t1 DEFAULT f1(); + SELECT r.a, r.b; +END; +$$ +DELIMITER ;$$ + +# Testing with no rows +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +SELECT f1()=ROW(1,'b1') AS c; +SELECT f1()=ROW(NULL,NULL) AS c; + +# Testing with one row +INSERT INTO t1 VALUES (1,'b1'); +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +SELECT f1()=ROW(1,'b1') AS c; +SELECT f1()=ROW(1,'') AS c; +SELECT f1()=ROW(2,'b1') AS c; +SELECT f1()=ROW(1,NULL) AS c; +SELECT f1()=ROW(NULL,'b1') AS c;
do one more SHOW CREATE FUNCTION here
+ +# Testing with two rows +INSERT INTO t1 VALUES (2,'b2'); +--error ER_SUBQUERY_NO_1_ROW +CALL p1(); +--error ER_OPERAND_COLUMNS +SELECT f1(); +--error ER_SUBQUERY_NO_1_ROW +SELECT f1()=ROW(1,'b1') AS c; + +DROP PROCEDURE p1; +DROP FUNCTION f1; +DROP TABLE t1; diff --git a/mysql-test/main/sp-anchor-type.test b/mysql-test/main/sp-anchor-type.test index 0e24ef900d8..fed53fd011a 100644 --- a/mysql-test/main/sp-anchor-type.test +++ b/mysql-test/main/sp-anchor-type.test @@ -767,3 +767,78 @@ BEGIN NOT ATOMIC END; $$ DELIMITER ;$$ + + +--echo # +--echo # Start of 11.3 tests +--echo # + +--echo # +--echo # MDEV-12252 ROW data type for stored function return values +--echo # + +--error ER_PARSE_ERROR +CREATE FUNCTION f1() RETURNS TYPE OF a RETURN 1; + +CREATE FUNCTION f1() RETURNS TYPE OF t1.a RETURN (SELECT min(a) FROM t1); +SHOW CREATE FUNCTION f1; +--error ER_NO_SUCH_TABLE +SELECT f1(); +CREATE TABLE t1 (b INT); +--error ER_BAD_FIELD_ERROR +SELECT f1(); +DROP TABLE t1; +DROP FUNCTION f1; + +CREATE DATABASE db1; +DELIMITER $$; +CREATE FUNCTION db1.f1() RETURNS TYPE OF db1.t1.a +BEGIN + RETURN (SELECT min(a) FROM t1); +END; +$$ +DELIMITER ;$$ +SHOW CREATE FUNCTION db1.f1; +--error ER_NO_SUCH_TABLE +SELECT db1.f1(); +CREATE TABLE db1.t1 (b TIME); +--error ER_BAD_FIELD_ERROR +SELECT db1.f1(); +DROP TABLE db1.t1; +CREATE TABLE db1.t1 (a TIME); +SELECT db1.f1(); +INSERT INTO db1.t1 VALUES ('10:20:30'); +SELECT db1.f1(); +DROP TABLE db1.t1; +DROP FUNCTION db1.f1; +DROP DATABASE db1; + +CREATE TABLE t1 (a TIME); +CREATE FUNCTION f1() RETURNS TYPE OF test.t1.a RETURN (SELECT min(a) FROM t1); +SHOW CREATE FUNCTION f1; +SELECT f1(); +DROP FUNCTION f1; +DROP TABLE t1; + +CREATE TABLE t1 (a TIME); +DELIMITER $$; +CREATE FUNCTION f1() RETURNS TYPE OF t1.a +BEGIN + RETURN (SELECT min(a) FROM t1); +END; +$$ +DELIMITER ;$$ +SHOW CREATE FUNCTION f1; +SELECT f1(); +CREATE TABLE t2 AS SELECT f1(); +SHOW CREATE TABLE t2; +SELECT * FROM t2; +DROP TABLE t2; +INSERT INTO t1 VALUES ('10:20:30'); +SELECT f1(); +CREATE TABLE t2 AS SELECT f1(); +SHOW CREATE TABLE t2; +SELECT * FROM t2; +DROP TABLE t2;
please, also try here DROP TABLE t1; CREATE TABLE t1 (a INT); INSERT INTO t1 VALUES (10); CREATE TABLE t2 AS SELECT f1(); the point is to change TYPE OF t1.a after the function has been successfully called, and thus is already in the sp cache.
+DROP FUNCTION f1; +DROP TABLE t1; diff --git a/mysql-test/suite/compat/oracle/t/sp-package.test b/mysql-test/suite/compat/oracle/t/sp-package.test index aefede41c8b..7c231d736c5 100644 --- a/mysql-test/suite/compat/oracle/t/sp-package.test +++ b/mysql-test/suite/compat/oracle/t/sp-package.test @@ -3109,3 +3109,79 @@ DROP TABLE t1; DROP FUNCTION f1_deterministic; DROP FUNCTION f2_not_deterministic; DROP PACKAGE pkg1; + +--echo # +--echo # MDEV-12252 ROW data type for stored function return values +--echo # + +--echo # +--echo # Testing fixed ROW type with package routines +--echo # + +DELIMITER $$; +CREATE PACKAGE pkg +AS + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)); + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))); + PROCEDURE p2(); +END; +$$ +CREATE PACKAGE BODY pkg +AS + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)) AS + BEGIN + RETURN ROW(1,'b1'); + END; + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))) AS + BEGIN + SELECT r.a, r.b; + END; + PROCEDURE p2() AS + BEGIN + CALL p1(f1()); + END; +END; +$$ +DELIMITER ;$$ +CALL pkg.p1(pkg.f1()); +CALL pkg.p2; +DROP PACKAGE pkg; + + +--echo # +--echo # Testing table%ROWTYPE with package routines +--echo #
now you can also add package tests outside of oracle mode
+ +CREATE TABLE t1 (a INT, b VARCHAR(32)); +INSERT INTO t1 VALUES (1,'b1'); +DELIMITER /; +CREATE PACKAGE pkg +AS + FUNCTION f1 RETURN t1%ROWTYPE; + PROCEDURE p1(r t1%ROWTYPE); + PROCEDURE p2; +END; +/ +CREATE PACKAGE BODY pkg +AS + FUNCTION f1 RETURN t1%ROWTYPE AS + r t1%ROWTYPE; + BEGIN + SELECT * INTO r FROM t1; + RETURN r; + END; + PROCEDURE p1(r t1%ROWTYPE) AS + BEGIN + SELECT r.a || ' ' || r.b; + END; + PROCEDURE p2 AS + BEGIN + p1(f1()); + END; +END; +/ +DELIMITER ;/ +CALL pkg.p1(pkg.f1()); +CALL pkg.p2; +DROP PACKAGE pkg; +DROP TABLE t1; diff --git a/sql/field.h b/sql/field.h index 7f1c243a180..7af069a9735 100644 --- a/sql/field.h +++ b/sql/field.h @@ -1373,6 +1374,20 @@ class Field: public Value_source in str and restore it with set() if needed */ virtual void sql_type(String &str) const =0; + void sql_type_for_sp_returns(String &str) const
This is oddly specific. Strange that we don't need to print the field type with charset/collation anywhere else, but for sp returns we do.
+ { + sql_type(str); + if (has_charset()) + { + str.append(STRING_WITH_LEN(" CHARSET ")); + str.append(charset()->cs_name); + if (Charset(charset()).can_have_collate_clause()) + { + str.append(STRING_WITH_LEN(" COLLATE ")); + str.append(charset()->coll_name); + } + } + } virtual void sql_rpl_type(String *str) const { sql_type(*str); } virtual uint size_of() const =0; // For new field inline bool is_null(my_ptrdiff_t row_offset= 0) const diff --git a/sql/field.cc b/sql/field.cc index 35be045620d..23b050de221 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2768,6 +2768,32 @@ bool Field_row::sp_prepare_and_store_item(THD *thd, Item **value) }
+uint Field_row::cols() const +{ + // The table with ROW members must be already instantiated + DBUG_ASSERT(m_table); + return m_table->s->fields; +} + + +void Field_row::sql_type(String &res) const
would be more logical to call this Field_row::sql_type_for_sp_returns()
+{ + res.set_ascii(STRING_WITH_LEN("row(")); + for (uint i= 0; i < m_table->s->fields; i++) + { + if (i > 0) + res.append(','); + Field *field= m_table->field[i]; + res.append(field->field_name); + res.append(' '); + StringBuffer<64> col; + field->sql_type_for_sp_returns(col); + res.append(col.to_lex_cstring()); + } + res.append(')'); +} + + /**************************************************************************** Functions for the Field_decimal class This is an number stored as a pre-space (or pre-zero) string diff --git a/sql/item.cc b/sql/item.cc index 54ad511cb6b..0a125752979 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -3001,10 +3003,31 @@ Item_sp::init_result_field(THD *thd, uint max_length, uint maybe_null, dummy_table->s->table_name= empty_clex_str; dummy_table->maybe_null= maybe_null;
+ if (m_sp->m_return_field_def.is_column_type_ref() && + m_sp->m_return_field_def.column_type_ref()-> + resolve_type_ref(thd, &m_sp->m_return_field_def)) + DBUG_RETURN(TRUE);
Here you overwrite old m_return_field_def value which was a column_type_ref with its actual type, don't you? What will happen if you change the column's type after the sp was already executed once?
+ if (!(sp_result_field= m_sp->create_result_field(max_length, name, dummy_table))) DBUG_RETURN(TRUE);
+ /* + In case of a ROW return type we need to create Item_field_row + on top of Field_row, and remember it in sp_result_item_field_row. + ROW members are later accessed using sp_result_item_field_row->addr(i), + e.g. when copying the function return value to a local variable. + For scalar return types no Item_field is needed around sp_result_field, + as the value is fetched directly from sp_result_field, + inside Item_func_sp::val_xxx() methods.
Sorry, I don't understand that. Why cannot you use Field_row here?
+ */ + if (Field_row *field_row= dynamic_cast<Field_row*>(sp_result_field)) + { + if (!(sp_result_item_field_row= + m_sp->m_return_field_def.make_item_field_row(thd, field_row))) + DBUG_RETURN(true); + } + if (sp_result_field->pack_length() > sizeof(result_buf)) { void *tmp; diff --git a/sql/sp.cc b/sql/sp.cc index ddaeeb8cb7a..7eab8304c31 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1091,18 +1136,19 @@ sp_returns_type(THD *thd, String &result, const sp_head *sp) bzero((char*) &share, sizeof(share)); table.in_use= thd; table.s = &share; - field= sp->create_result_field(0, 0, &table); - field->sql_type(result); + field= create_result_field(0, 0, &table);
- if (field->has_charset()) + if (m_return_field_def.is_row()) { - result.append(STRING_WITH_LEN(" CHARSET ")); - result.append(field->charset()->cs_name); - if (Charset(field->charset()).can_have_collate_clause()) - { - result.append(STRING_WITH_LEN(" COLLATE ")); - result.append(field->charset()->coll_name); - } + Field_row *field_row= dynamic_cast<Field_row*>(field); + if (!field_row->row_create_fields( + thd, m_return_field_def.row_field_definitions())) + field->sql_type(result);
when is this used? you're creating a TABLE and Field's - only to print types and charsets. This might be tolerable, but only if it's not needed often
+ } + else + { + DBUG_ASSERT(m_return_field_def.type_handler()->is_scalar_type()); + field->sql_type_for_sp_returns(result); }
delete field; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 5977e309ae4..f11e4f89466 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -18347,7 +18338,50 @@ sp_parameters: ;
sf_returned_type_clause: - RETURNS_SYM sf_return_type + RETURNS_SYM + { + LEX *lex= Lex;
you still do this lex=Lex? :) it became obsolete, like, 15 years ago. If not more.
+ lex->init_last_field(&lex->sphead->m_return_field_def, + &empty_clex_str); + } + sf_return_type + ; + +sf_return_type: + field_type + { + if (unlikely(Lex->sf_return_fill_definition($1))) + MYSQL_YYABORT; + } + | ROW_SYM row_type_body + { + if (Lex->sf_return_fill_definition_row($2)) + MYSQL_YYABORT; + } + | ROW_SYM TYPE_SYM OF_SYM ident + { + if (Lex->sf_return_fill_definition_rowtype_of( + Qualified_column_ident(&$4))) + MYSQL_YYABORT; + } + | ROW_SYM TYPE_SYM OF_SYM ident '.' ident + { + if (Lex->sf_return_fill_definition_rowtype_of( + Qualified_column_ident(&$4, &$6))) + MYSQL_YYABORT; + } + | TYPE_SYM OF_SYM ident '.' ident + { + if (Lex->sf_return_fill_definition_type_of( + Qualified_column_ident(&$3, &$5))) + MYSQL_YYABORT; + } + | TYPE_SYM OF_SYM ident '.' ident '.' ident + { + if (Lex->sf_return_fill_definition_type_of( + Qualified_column_ident(thd, &$3, &$5, &$7))) + MYSQL_YYABORT; + } ;
package_implementation_item_declaration: