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
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
