Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coprocessor: allow invalid dates SQL mode #4100

Open
gregwebs opened this Issue Jan 23, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@gregwebs
Copy link
Collaborator

gregwebs commented Jan 23, 2019

Context: in TiDB we added the SQL mode ALLOW_INVALID_DATES: pingcap/tidb#8263

This is tested to work for INSERT and I think select works for mocktikv. However, in my testing it does not work with the TiKV co-processor.

Bug Report

Docker images:

  • pingcap/tikv:v2.1.0
  • a docker build of the latest of the 2.1 release branch

What did you do?

From a TiDB MySQL client:

mysql> CREATE TABLE d1 (d DATE);
Query OK, 0 rows affected (0.56 sec)

mysql> SET sql_mode='STRICT_TRANS_TABLES,ALLOW_INVALID_DATES';
Query OK, 0 rows affected (0.17 sec)

mysql> INSERT INTO d1 VALUES ('2010-00-01');
Query OK, 1 row affected (0.52 sec)

mysql> SELECT * from d1 WHERE d='2010-00-01';
ERROR 1105 (HY000): other error: eval error code: 1105 msg: "Other(StringError(\"[src/coprocessor/codec/mysql/time/mod.rs:100]: \\'2010-0-1 0:0:0.000000000\\' is not a valid datetime in specified time zone\"))

What did you expect to see?

No error

What did you see instead?

The above error

@gregwebs

This comment has been minimized.

Copy link
Collaborator Author

gregwebs commented Jan 23, 2019

@breeswish this is listed as Easy now, but I wouldn't know how to propagate SQL modes in the coprocessor.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Jan 23, 2019

@gregwebs SQL modes are already pushed down in Coprocessor. I think it is fine to just check the corresponding flag's existence.

@gregwebs

This comment has been minimized.

Copy link
Collaborator Author

gregwebs commented Jan 23, 2019

I am told the docs for the coprocessor are here: https://pingcap.com/blog/adding-built-in-functions-to-tikv/
What would make coprocessor changes easier for me would be to be able to find the docs (it doesn't show up in a Google search) linked from the source code.
Still though from looking at the docs I would know how to add a new function but not how to alter the existing source for this case.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Jan 23, 2019

Right, I agree with you @gregwebs When someone is going to work on this, I can give some help on how to get started.

@breeswish breeswish added the D: Mentor label Jan 23, 2019

@gregwebs

This comment has been minimized.

Copy link
Collaborator Author

gregwebs commented Feb 20, 2019

I looked into this in more detail with help from @brson. @brson also mentioned that there were some existing plans to alter the Time data type that may need to be taken into account. This task will likely require some refactoring and benchmarking, so it seems more than the simple fix that I have time available for right now. But we can write down our notes.

In mod.rs the failure happens when the time is given to chrono to construct as a valid time. We need to either skip chrono construction when the invalid date mode is on, much like we do now for zero times, or do invalid date checking after the chrono construction fails. This means mod.rs needs to be passed the relevant SQL mode flags, or probably better we need to call a different entry point when the SQL mode flags are on.

After constructing a valid time, mod.rs returns a custom Time struct. This struct has a field TimeType that is an enumeration. TimeType would need to be extended to support an invalid time, which would mean that it would now hold data, which would make the Time struct bigger. An alternative that I favor would be to refactor to make Time itself the enumeration and remove the TimeType field from Time: this should keep the Time struct the same size.

The sql modes are taken in as a bit-shifted field and we would need to add a bitshift for invalid dates in src/coprocessor/dag/expr/ctx.rs

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Feb 20, 2019

I guess this can be just a flag marking whether or not current time is invalid, plus a flag marking whether or not invalid time should be errors.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 22, 2019

re the size of Time the issue I was actually thinking of was to reduce the size of Duration: #4025

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 26, 2019

Although there are unknowns, getting started on this looks pretty straightforward:

  • add a mode_allow_invalid_dates method to EvalConfig in src/coprocessor/dag/expr/ctx.rs, similar to the other mode_* functions. Add the corresponding MODE_* constant.
  • you shouldn't need to modify the code that creates EvalConfig (seemingly in dag_handler) because TiDB is already sending the correct mode flag, which should be making it to EvalConfig.
  • modify builtin_time such that inserts and queries of invalid times conform to mysql. This is where the unknowns and experimentation will be. (offhand, it looks like it's possible that tikv is also not handling zero representations right). In the process you'll need to change the representation of Time in src/coprocessor/codec/mysql/time/mod.rs to support invalid times; and probably to change the various eval time functions that builtin_time.rs depends on.
  • write tests (ask someone else exactly how to do that.

@brson brson added D: Medium and removed D: Easy labels Feb 26, 2019

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 26, 2019

@breeswish can mentor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.