-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HIVE-25306: Move Date and Timestamp parsing from ResolverStyle.LENIENT to ResolverStyle.STRICT #2445
Conversation
@zabetak Could you please review the PR? |
c4f1e0b
to
5ad7510
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work.
This is going to change the behavior for customers. Previously, the queries which used to work fine, will start throwing exception now. |
ql/src/test/queries/clientpositive/materialized_view_rewrite_in_between.q
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
Outdated
Show resolved
Hide resolved
I'm also in favour of keeping this change within a config. |
@mattmccline-microsoft @ashish-kumar-sharma @sankarh I added a few comments in the JIRA since I think we should agree there before doing a line by line review. |
common/src/test/org/apache/hive/common/util/TestTimestampParser.java
Outdated
Show resolved
Hide resolved
common/src/test/org/apache/hive/common/util/TestTimestampParser.java
Outdated
Show resolved
Hide resolved
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFAddMonths.java
Show resolved
Hide resolved
common/src/java/org/apache/hive/common/util/TimestampParser.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I will merge this now!
…sh Kumar Sharma, reviewed by Adesh Kumar Rao, Matt McCline, Sankar Hariappan, Stamatis Zampetakis) With this change parsing string literals to dates/timestamps does not tolerate dates which do not conform exactly to the expected syntax and does not compensate for months, days, etc., out of the usual ranges. When the literal cannot be parsed the null value is returned adopting the default MySQL behavior. This is a breaking change. Closes apache#2445
…sh Kumar Sharma, reviewed by Adesh Kumar Rao, Matt McCline, Sankar Hariappan, Stamatis Zampetakis) With this change parsing string literals to dates/timestamps does not tolerate dates which do not conform exactly to the expected syntax and does not compensate for months, days, etc., out of the usual ranges. When the literal cannot be parsed the null value is returned adopting the default MySQL behavior. This is a breaking change. Closes apache#2445
What changes were proposed in this pull request?
Currently Date.java and Timestamp.java use DateTimeFormatter for parsing to convert the date/timpstamp from int,string,char etc to Date or Timestamp.
Default DateTimeFormatter which use ResolverStyle.LENIENT which mean date like "1992-13-12" is converted to "2000-01-12",
Moving DateTimeFormatter which use ResolverStyle.STRICT which mean date like "1992-13-12" is not be converted instead NULL is return.
Why are the changes needed?
ResolverStyle.LENIENT to ResolverStyle.STRICT
Does this PR introduce any user-facing change?
No
How was this patch tested?
UTs and QTs added as part of PR