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