Hi! Here is the review for MDEV-6017 Add support for Indexes on Expressions commit c1e7574c799163f5d8a26ca8e0f91af6c91f7077 Instead of adding bool function virtual bool vcol_subst_analyzer(uchar **) { return false; } Why not add Item_base_t flag: IS_SIMPLE_BOOL_FUNC This takes one bit instead of increasing ALL created item base structures with 8 bytes. As we have 100+ items, adding a virtual function increases code space with 1K or more + the functions. You can also replace vcol_subst_analyzer() with a top level function: bool Item::vcol_subst_analyzer/( { return (type() == Item::FUNC_ITEM && (((Item_func*) this)->bitmap_bit() & (BITMAP_EQ | BITMAP_EQUAL | BITMAP_LT | BITMAP_GT | BITMAP_LE | BITMAP_GE | BITMAP_BETWEEN | BITMAP_IN | BITMAP_ISNULL | BITMAP_ISNOTNULL ) } For this to work, you would have to add the last two bitmaps to enum Bitmap in item_func.h @@ -3272,6 +3292,7 @@ class Item_cond :public Item_bool_func bool excl_dep_on_table(table_map tab_map) override; bool excl_dep_on_grouping_fields(st_select_lex *sel) override; + bool vcol_subst_analyzer(uchar **) override { return true; } I assume you need the above to be able to traverse AND and OR expressions for WHERE. Please add a comment that this is the intent. If you decide to use the bitmaps, add an explanation why AND and OR is in the vcol_subst_analyzer list. ---- +void collect_indexed_vcols_for_table(TABLE *tbl, List<Field> *vcol_fields) +{ Please use table instead of tbl. We use table as a varible much more than tbl: ((/my/maria-10.5/sql)) grep tbl *.* | wc 1143 5371 73297 ((/my/maria-10.5/sql)) grep table *.* | wc 40886 236754 2832202 + for (uint i=0; i < tbl->s->keys; i++) + { + // note: we could also support histograms here + if (!tbl->keys_in_use_for_query.is_set(i)) + continue; I wonder if it would be a good idea to extend bitmap to have an iterator like we have for tables where we only loop over the set bits. From sql_bitmap.h: class Table_map_iterator { ulonglong bmp; public: Table_map_iterator(ulonglong t): bmp(t){} uint next_bit() { if (!bmp) return BITMAP_END; uint bit= my_find_first_bit(bmp); bmp &= ~(1ULL << bit); return bit; } int operator++(int) { return next_bit(); } enum { BITMAP_END= 64 }; }; Would be quite trival to do also for keys. This would help tables with many keys and also would allows us to remove the 'call' to if (!tbl->keys_in_use_for_query.is_set(i)) We could also do an optimized version where we implement bitmap_get_next_set() where we do not have to reset the previous bits for each call. --------- + if (field->vcol_info) + vcol_fields->push_back(field); You have to take care of error handling here. Please change the functions collect_indexed_vcols_for_table(), collect_indexed_vcols_for_join() and other functions that calls push_back() to bool and return 1 in case of error. + item->top_level_compile(ctx->thd, + &Item::vcol_subst_analyzer, &yes, + &Item::vcol_subst_transformer, (uchar*)ctx); You also have to check the return of the above for errors. Another option is to just check thd->fatal_error() as this will be set in case of OOM. .... + { + if (auto nested_join= table->nested_join) + subst_vcols_in_join_list(ctx, &nested_join->join_list); PLEASE do not use auto. I want to know the type while reading and debugging the code! auto can also introduce bugs if a type changes. + @todo: this cannot handle VIEW columns currently. View columns are wrapped + in Item_direct_view_ref objects which do not comsider themselves equal to + Item_field objects they're wrapping. Please add this also to the commit comment. + /* + Do not do the substitution if the datatypes do not match. + Virtual column's datatype can be different from the expression, for + example: + + col3 INT AS (CONCAT(col1, col2)) + + Do not allow substitutions that change the semantics of comparison. + At the moment, we require that datatypes are the same. + This probably could be relaxed. + + For strings: we allow two cases: + - vcol_expr and vcol_field have the same collation. + - vcol_field has the same collation as the comparison collation. + + (Note: MySQL calls resolve_type() after it has done the substitution. + This can potentially update the comparator. The idea is that this + shouldn't be necessary as we do not want to change the comparator. + Changing the comparator will change the semantics of the condition, + our point is that this must not happen) + */ Move to a function comment. It is hard to read read the function code with this long comment in the middle. + if (vcol_expr->type_handler_for_comparison() != + vcol_field->type_handler_for_comparison() || + vcol_expr->maybe_null() != vcol_field->maybe_null()) + fail_cause="type mismatch"; Why cannot we handle this case if vcol_expr->maybe_null is 0 and vcol_field->maybe_null() is set? In this case we know that the vcol_field's we are going to encounter can never be null, which should satisfy our requirements. + Item_field *itf= new (thd->mem_root) Item_field(thd, vcol_field); + if (!itf) + return false; Better to return true in case of errors, like we do in most of the other code. Remove and a blank line instead + //Item *const_expr; ... remove + //const_expr= args[0]; ----- +Item* Item_func_in::vcol_subst_transformer(THD *thd, uchar *arg) +{ + Vcol_subst_context *ctx= (Vcol_subst_context*)arg; + if (!compatible_types_scalar_bisection_possible()) Add comment /* Check that all arguments inside IN() are constants */ Regards, Monty