findepi commented on code in PR #12227:
URL: https://github.com/apache/datafusion/pull/12227#discussion_r1739338876
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -47,22 +57,39 @@ impl ToDateFunc {
match args.len() {
1 => handle::<Date32Type, _, Date32Type>(
args,
+ // Although not being speficied, previous implementations have
supported parsing date values that are followed
+ // by a time part separated either a whitespace ('2023-01-10
12:34:56.000') or a 'T' ('2023-01-10T12:34:56.000').
Review Comment:
- Was the previous implementation accepting `<date>T<time>` or
`<date>T<anything>`? The comment suggests the former, the code now accepts the
latter
- if this is desirable behavior, we should document it (and then we can use
a different code comment wording -- not referring to previous implementation as
the one constraining us for ever). If this isn't desirable behavior, we should
fix it. In any case it might be best to address this as a follow-up issue.
@comphead WDYT?
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -47,22 +57,39 @@ impl ToDateFunc {
match args.len() {
1 => handle::<Date32Type, _, Date32Type>(
args,
+ // Although not being speficied, previous implementations have
supported parsing date values that are followed
+ // by a time part separated either a whitespace ('2023-01-10
12:34:56.000') or a 'T' ('2023-01-10T12:34:56.000').
+ // Parsing these strings with "%Y-%m-%d" will fail. Therefore
the string is split and the first part is used for parsing.
|s| {
- string_to_timestamp_nanos_shim(s)
- .map(|n| n / (1_000_000 * 24 * 60 * 60 * 1_000))
- .and_then(|v| {
- v.try_into().map_err(|_| {
- internal_datafusion_err!("Unable to cast to
Date32 for converting from i64 to i32 failed")
- })
- })
+ let string_to_parse;
+ if s.find('T').is_some() {
+ string_to_parse = s.split('T').next();
+ } else {
+ string_to_parse = s.split_whitespace().next();
+ }
+ if string_to_parse.is_none() {
+ return Err(internal_datafusion_err!(
Review Comment:
please try to use `internal_err!` per
https://github.com/apache/datafusion/pull/12227#discussion_r1738902790
##########
datafusion/functions/src/datetime/to_date.rs:
##########
@@ -47,22 +57,39 @@ impl ToDateFunc {
match args.len() {
1 => handle::<Date32Type, _, Date32Type>(
args,
+ // Although not being speficied, previous implementations have
supported parsing date values that are followed
+ // by a time part separated either a whitespace ('2023-01-10
12:34:56.000') or a 'T' ('2023-01-10T12:34:56.000').
+ // Parsing these strings with "%Y-%m-%d" will fail. Therefore
the string is split and the first part is used for parsing.
|s| {
- string_to_timestamp_nanos_shim(s)
- .map(|n| n / (1_000_000 * 24 * 60 * 60 * 1_000))
- .and_then(|v| {
- v.try_into().map_err(|_| {
- internal_datafusion_err!("Unable to cast to
Date32 for converting from i64 to i32 failed")
- })
- })
+ let string_to_parse;
+ if s.find('T').is_some() {
+ string_to_parse = s.split('T').next();
+ } else {
+ string_to_parse = s.split_whitespace().next();
+ }
+ if string_to_parse.is_none() {
+ return Err(internal_datafusion_err!(
+ "Cannot cast emtpy string to Date32"
+ ));
+ }
+
+ // For most cases 'Date32Type::parse' would also work
here, as it semantically parses
+ // assuming a YYYY-MM-DD format. However
'Date32Type::parse' cannot handle negative
+ // values for year (e.g. years in BC).
Date32Type::parse_formatted can handle also these.
+ match
Date32Type::parse_formatted(string_to_parse.unwrap(), "%Y-%m-%d") {
+ Some(v) => Ok(v),
Review Comment:
formatting seems off? typically `match` options are indented
(`cargo fmt` seems to be fine either way, not sure why)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
