On Mon, Dec 1, 2025 at 1:34 PM 'Alexander Barkov' via Michael Widenius
<[email protected]> wrote:
>
> Hello Monty,
>
> The patch looks good. I noticed a few things though.
>
> - Oracle's TO_DATE() always returns Oracle's DATE data type.
> Which in MariaDB is DATETIME(0).
> So in MariaDB, TO_DATE() should never return MariaDB's DATE
> for compatibility purposes:
> Oracle does not have MariaDB style DATE data type.
Fixed so that TO_DATE returns datetime or datetime with usec.
> - Oracle's TO_DATE() does not support the FF format specifier.
> Oracle returns an error on attempts to use it.
> To be strictly compatible, MariaDB could also return an error.
> Otherwise, if we want to support FF, it should be clearly documented.
Commit updated that MariaDB supports FF while Oracle does not.
> - The function uses session variables @@timezone, @@timestamp,
> @@lc_time_names.
> Therefore it's not deterministic. It should not be allowed as a
> vcol virtual or vcol indexed expression.
We could detect this based on the format. A problem is that when
check_vcol_func_process is called, fix_fields is not yet run, so we
cannot check the state which is set by fix_length_and_dec() :(
Added marking TO_DATE() not deterministic.
> create or replace table t1 (
> c1 varchar(10),
> c2 datetime as (to_date(c1,'MM')) virtual,
> key(c2));
>
> - The same reason (the use of @@timezone, @@timestamp, @lc_time_names)
> makes it not deterministic for replication purposes.
>
> It should a have a five-argument version so when put into
> the binlog it prints itself in a deterministic way:
After discussion on slack we concluded that the timezone is already in
binlog and adding a test cases with binlog is not needed. (I did a
test to verfify this)
> For example:
> TO_DATE('10','MM', '', 'NLS_DATE_LANGUAGE = German', '2001-02-03')
I have added support for NLS_DATE_LANGUAGE in my latest patch in
bb-main-monty
<cut>
> > FF[1-6] Fractional seconds
>
> Supporting FF is not compatible with Oracle.
It is an extension and is marked as such in my latest commit.
Note that we *are* compatible with Oracle if MariaDB returns the same
answer for all cases where Oracle returns an answer (and not an error).
I don't think we should be limited to things Oracle does not support or
that does not make sense for MariaDB.
> > index 0be8cf61591..1fe2fb8ee59 100644
> > --- a/sql/item_func.h
> > +++ b/sql/item_func.h
> > @@ -874,6 +874,8 @@ class Item_handled_func: public Item_func
> > :Item_func(thd, a), m_func_handler(NULL) { }
> > Item_handled_func(THD *thd, Item *a, Item *b)
> > :Item_func(thd, a, b), m_func_handler(NULL) { }
> > + Item_handled_func(THD *thd, Item *a, Item *b, Item *c)
> > + :Item_func(thd, a, b, c), m_func_handler(NULL) { }
> > void set_func_handler(const Handler *handler)
> > {
> > m_func_handler= handler;
> > @@ -918,6 +920,12 @@ class Item_handled_func: public Item_func
> > {
> > return m_func_handler->val_native(thd, this, to);
> > }
> > + virtual bool get_date_common(THD *thd, MYSQL_TIME *ltime,
> > + date_mode_t fuzzydate, timestamp_type)
> > + {
> > + DBUG_ASSERT(0);
> > + return 0;
> > + }
>
> Please don't add this virtual method.
>
> Item_handled_func is a generic class, for functions returning any data type,
> not only DATE/TIME. It must stay as clean as possible and should not contain
> extra data type specific methods.
> Adding get_date_common() breaks this idea.
>
> I will think of another ways to avoid the cast in the future.
Having casts in the original code was already horribly wrong.
My patch cleans this up.
Adding another overlay to all the functions that needs
get_date_common() from Item_handled_func
would be an even worse solution..
You can fix this at the same time you decide to clean up the other
usage it item->get_date_common().
> For now, please don't use "virtual" with this method.
> You need to make a new class Func_handler_to_date which can
> derive from Func_handler_str_to_date_datetime_sec.
> It should override get_date() to call the non-virtual get_date_common()
> of the owner.
Please no more classes if not really nesscary.
We already have too many classes derived from other classes
just to solve on problem. The code is now already very hard
to read and understand.
We have at some point look at the item time functions
and see if there is some way to remove some of
the multiple inheritance we are now using in so many places.
> > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc
<cut>
> > +
> > +static bool
> > +extract_oracle_date_time(THD *thd, uint16 *format,
> Please add "const" to "format".
Added
<cut>
> > +static const Item_handled_func::Handler *
> > +get_parsed_result_type(PARSE_TYPE_FLAGS type)
> > +{
> > + /* If YEAR, MONTH or DAY is used, set all date bits */
> > + if (type & PARSE_TYPE_DATE)
> > + type= (PARSE_TYPE_FLAGS) (type | PARSE_TYPE_DATE);
> > + if (type & PARSE_TYPE_SUBSECONDS)
> > + return &func_handler_str_to_date_datetime_usec;
> > + if (type & PARSE_TYPE_TIME)
> > + return &func_handler_str_to_date_datetime_sec;
> > + return &func_handler_str_to_date_date;
> > +}
> > +
>
> The above is not Oracle compatible.
> It should never use &func_handler_str_to_date_date.
Fixed to only return func_handler_str_to_date_datetime_sec or
func_handler_str_to_date_datetime_usec;
> > +bool Item_func_to_date::fix_length_and_dec(THD *thd)
> > +{
> > + PARSE_TYPE_FLAGS format_flags= PARSE_TYPE_NONE;
> > +
> > + locale= thd->variables.lc_time_names;
> > +
> > + if (!args[0]->type_handler()->is_traditional_scalar_type() ||
> > + !args[1]->type_handler()->is_traditional_scalar_type() ||
> > + (arg_count == 33 &&
>
> Why 33? I guess it should be:
> (arg_count > 2 &&
Should be == 3. Fikxed (NLS parameter is not included in arg_count)
> Please fix and add a test case covering arg_count>2.
There are now tests for that.
>
> > + !args[2]->type_handler()->is_traditional_scalar_type()))
> > + {
> > + my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(0),
> > + args[0]->type_handler()->name().ptr(),
> > + args[2]->type_handler()->name().ptr(), func_name());
> > + return TRUE;
> > + }
> > + if (agg_arg_charsets(collation, args, arg_count,
> MY_COLL_ALLOW_CONV, 1))
> > + return TRUE;
> > + if (collation.collation->mbminlen > 1)
> > + internal_charset= &my_charset_utf8mb4_general_ci;
>
>
> I think internal_charset should always be utf8, because locale data is utf8.
> I have not tested, but from my understanding it won't be able to compare
> month names with non-ascii characters properly, say latin1 month name
> coming from the subject string in args[0] versus the month name in the
> locale data.
The above code is copied from
Item_func_str_to_date::fix_length_and_dec(THD *thd)
Shouldn't we use similar rules for str_to_date() and to_date()?
How do you suggest we go forwards with this?
> > +
> > + set_maybe_null();
> > + set_func_handler(&func_handler_str_to_date_datetime_usec);
> > +
> > + if ((const_item= args[1]->const_item()))
> > + {
>
> Can you please rename the member const_item to const_format?
> const_item looks confusing.
done
> > + if (!(format_flags= get_format()))
> > + return true;
>
> The above looks misleading. I suggest:
>
> if ((format_flags= get_format()) == PARSE_TYPE_NONE)
> return true;
done
> > +bool Item_func_to_date::get_date_common(THD *thd, MYSQL_TIME *ltime,
> > + date_mode_t fuzzydate,
> > + timestamp_type tstype)
> > +{
> > + StringBuffer<64> val_string;
> > + String *val;
> > +
> > + val= args[0]->val_str(&val_string, &subject_converter,
> internal_charset);
> > + if (args[1]->null_value)
> > + goto error;
>
> Why args[1]->null_value it tested without calling args[1]->val_str() ?
Old code. Removed
>
> > + if (args[0]->null_value)
> > + {
> > + if (arg_count == 2)
> > + goto error;
> > + val= args[2]->val_str(&val_string, &subject_converter,
> internal_charset);
> > + if (args[2]->null_value)
> > + goto error;
> > + }
>
>
> Looks good. According to Oracle's docs, args[2], which is return_value
> from the `DEFAULT return_value ON CONVERSION ERROR` clause, is converted
> to DATE using the same method it uses for args[0] (the subject parameter).
> So it should be exactly args[2]->val_str() rather than args[2]->get_date().
> Thanks for noticing this.
>
> Can you please add a comment here?
>
> Oracle's behavior is strange.
Comment added. We also already have test case for this
>
> > +
> > + if (!const_item && !get_format())
> > + goto error;
>
> Again, !get_format() looks misleading. I suggest:
>
>
> if (!const_item && (get_format() == PARSE_TYPE_NONE))
> goto error;
done
> > diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h
> > +#define PARSE_TYPE_DATE (PARSE_TYPE_YEAR | PARSE_TYPE_MONTH |
> PARSE_TYPE_DAY)
> > +
> > +class Item_func_to_date :public Item_handled_func
> > +{
> > + bool const_item;
>
>
> Please rename it to const_format.
done
> > +++ b/sql/sql_yacc.yy
> > @@ -827,6 +826,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t
> *yystacksize);
> > %token <kwd> CONTAINS_SYM /* SQL-2003-N */
> > %token <kwd> CONTEXT_SYM
> > %token <kwd> CONTRIBUTORS_SYM
> > +%token <kwd> CONVERSION_SYM
> > %token <kwd> CPU_SYM
> > %token <kwd> CUBE_SYM /* SQL-2003-R */
> > %token <kwd> CURRENT_SYM /* SQL-2003-R */
> > @@ -1142,6 +1142,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b,
> size_t *yystacksize);
> > %token <kwd> TRANSACTION_SYM
> > %token <kwd> TRANSACTIONAL_SYM
> > %token <kwd> THREADS_SYM
> > +%token <kwd> TO_DATE /* PLSQL function */
> > %token <kwd> TRIGGERS_SYM
> > %token <kwd> TRIM_ORACLE
> > %token <kwd> TRUNCATE_SYM
>
>
> Don't forget to add the two new keywords into the rule
> "keyword_func_sp_var_and_label", so they are still allowed as
> table, column names etc, for backward compatibility.
done
It caused an error:
/my/maria-main/sql/yy_mariadb.yy: error: shift/reduce conflicts: 64
found, 63 expected
We agreed on slack to record 64 for now.
Regards,
Monty
_______________________________________________
developers mailing list -- [email protected]
To unsubscribe send an email to [email protected]
