MDEV-30395 Wrong result with semijoin and Federated as outer table

The problem was that federated engine does not support comparable rowids
which was not taken into account by semijoin code.

Fixed by checking that we don't use semijoin with tables that does not
support comparable rowids.

Other things:
- Fixed some typos in the code comments

6 files changed, 89 insertions, 7 deletions

diff --git a/mysql-test/suite/federated/federatedx.result b/mysql-test/suite/federated/federatedx.result
index c18665e4d99..49deff81c4c 100644
--- a/mysql-test/suite/federated/federatedx.result
+++ b/mysql-test/suite/federated/federatedx.result
@@ -2325,6 +2325,38 @@ DROP TABLE federated.t1;
 connection slave;
 DROP TABLE federated.t1;
 connection default;
+#
+# MDEV-30395 Wrong result with semijoin and Federated as outer table
+#
+create server s foreign data wrapper mysql options (host "127.0.0.1", database "test", user "root", port MASTER_PORT);
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (3),(2),(3);
+CREATE TABLE t2 (pk INT PRIMARY KEY);
+INSERT INTO t2 VALUES (1),(2),(3),(4);
+set @save_optimizer_switch=@@optimizer_switch;
+set optimizer_switch="materialization=off";
+CREATE TABLE t2_fed ENGINE=FEDERATED CONNECTION='s/t2';
+explain SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	PRIMARY	t2_fed	ALL	NULL	NULL	NULL	NULL	4	Using where
+2	DEPENDENT SUBQUERY	t1	ALL	NULL	NULL	NULL	NULL	3	Using where
+SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+pk
+2
+3
+SET optimizer_switch='semijoin=off';
+explain SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	PRIMARY	t2_fed	ALL	NULL	NULL	NULL	NULL	4	Using where
+2	DEPENDENT SUBQUERY	t1	ALL	NULL	NULL	NULL	NULL	3	Using where
+SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+pk
+2
+3
+DROP TABLE t2_fed, t1, t2;
+set @@optimizer_switch=@save_optimizer_switch;
+DROP SERVER s;
+# End of 10.5 tests
 connection master;
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
diff --git a/mysql-test/suite/federated/federatedx.test b/mysql-test/suite/federated/federatedx.test
index 51d34298626..7e5a335b786 100644
--- a/mysql-test/suite/federated/federatedx.test
+++ b/mysql-test/suite/federated/federatedx.test
@@ -2060,4 +2060,34 @@ connection slave;
 DROP TABLE federated.t1;
 connection default;
 
+--echo #
+--echo # MDEV-30395 Wrong result with semijoin and Federated as outer table
+--echo #
+
+
+--replace_result $MASTER_MYPORT MASTER_PORT
+eval create server s foreign data wrapper mysql options (host "127.0.0.1", database "test", user "root", port $MASTER_MYPORT);
+
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (3),(2),(3);
+CREATE TABLE t2 (pk INT PRIMARY KEY);
+INSERT INTO t2 VALUES (1),(2),(3),(4);
+
+set @save_optimizer_switch=@@optimizer_switch;
+set optimizer_switch="materialization=off";
+
+CREATE TABLE t2_fed ENGINE=FEDERATED CONNECTION='s/t2';
+explain SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+SET optimizer_switch='semijoin=off';
+explain SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+SELECT * FROM t2_fed WHERE pk IN ( SELECT a FROM t1 );
+
+DROP TABLE t2_fed, t1, t2;
+set @@optimizer_switch=@save_optimizer_switch;
+
+DROP SERVER s;
+
+--echo # End of 10.5 tests
+
 source include/federated_cleanup.inc;
diff --git a/sql/handler.h b/sql/handler.h
index 75cd88b8013..3191c408e56 100644
--- a/sql/handler.h
+++ b/sql/handler.h
@@ -356,9 +356,9 @@ enum chf_create_flags {
   Rowid's are not comparable. This is set if the rowid is unique to the
   current open handler, like it is with federated where the rowid is a
   pointer to a local result set buffer. The effect of having this set is
-  that the optimizer will not consirer the following optimizations for
+  that the optimizer will not consider the following optimizations for
   the table:
-  ror scans or filtering
+  ror scans, filtering or duplicate weedout
 */
 #define HA_NON_COMPARABLE_ROWID (1ULL << 60)
 
diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc
index 3b3f9e56606..d516dc02b90 100644
--- a/sql/opt_subselect.cc
+++ b/sql/opt_subselect.cc
@@ -664,6 +664,17 @@ int check_and_do_in_subquery_rewrites(JOIN *join)
         DBUG_RETURN(-1);
       }
     }
+    /* Check if any table is not supporting comparable rowids */
+    {
+      List_iterator_fast<TABLE_LIST> li(select_lex->outer_select()->leaf_tables);
+      TABLE_LIST *tbl;
+      while ((tbl = li++))
+      {
+        TABLE *table= tbl->table;
+        if (table && table->file->ha_table_flags() & HA_NON_COMPARABLE_ROWID)
+          join->not_usable_rowid_map|= table->map;
+      }
+    }
 
     DBUG_PRINT("info", ("Checking if subq can be converted to semi-join"));
     /*
@@ -683,8 +694,11 @@ int check_and_do_in_subquery_rewrites(JOIN *join)
         9. Parent select is not a table-less select
         10. Neither parent nor child select have STRAIGHT_JOIN option.
         11. It is first optimisation (the subquery could be moved from ON
-        clause during first optimisation and then be considered for SJ
-        on the second when it is too late)
+            clause during first optimisation and then be considered for SJ
+            on the second when it is too late)
+        12. All tables supports comparable rowids.
+            This is needed for DuplicateWeedout strategy to work (which
+            is the catch-all semi-join strategy so it must be applicable).
     */
     if (optimizer_flag(thd, OPTIMIZER_SWITCH_SEMIJOIN) &&
         in_subs &&                                                    // 1
@@ -699,7 +713,8 @@ int check_and_do_in_subquery_rewrites(JOIN *join)
         !((join->select_options |                                     // 10
            select_lex->outer_select()->join->select_options)          // 10
           & SELECT_STRAIGHT_JOIN) &&                                  // 10
-        select_lex->first_cond_optimization)                          // 11
+        select_lex->first_cond_optimization &&                        // 11
+        join->not_usable_rowid_map == 0)                              // 12
     {
       DBUG_PRINT("info", ("Subquery is semi-join conversion candidate"));
 
@@ -3544,6 +3559,9 @@ bool Duplicate_weedout_picker::check_qep(JOIN *join,
       }
       else
       {
+        /* Ensure that table supports comparable rowids */
+        DBUG_ASSERT(!(p->table->table->file->ha_table_flags() & HA_NON_COMPARABLE_ROWID));
+
         sj_outer_fanout= COST_MULT(sj_outer_fanout, p->records_read);
         temptable_rec_size += p->table->table->file->ref_length;
       }
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 7bbe5c6a237..a721899a8be 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -2309,7 +2309,7 @@ JOIN::optimize_inner()
   /*
     We have to remove constants and duplicates from group_list before
     calling make_join_statistics() as this may call get_best_group_min_max()
-    which needs a simplfied group_list.
+    which needs a simplified group_list.
   */
   if (group_list && table_count == 1)
   {
diff --git a/sql/sql_select.h b/sql/sql_select.h
index d8b0ed290db..807b4115fec 100644
--- a/sql/sql_select.h
+++ b/sql/sql_select.h
@@ -1238,6 +1238,8 @@ public:
   table_map outer_join;
   /* Bitmap of tables used in the select list items */
   table_map select_list_used_tables;
+  /* Tables that has HA_NON_COMPARABLE_ROWID (does not support rowid) set */
+  table_map not_usable_rowid_map;
   ha_rows  send_records,found_records,join_examined_rows;
 
   /*
@@ -1550,7 +1552,7 @@ public:
     table_count= 0;
     top_join_tab_count= 0;
     const_tables= 0;
-    const_table_map= found_const_table_map= 0;
+    const_table_map= found_const_table_map= not_usable_rowid_map= 0;
     aggr_tables= 0;
     eliminated_tables= 0;
     join_list= 0;