Invalid trailing content after Z in timezone is ignored

[alamb]

Describe the bug
Timstamps like 2023-12-05T21:58:10.45ZZTOP which are invalid (they have trailing content after the Z) should error when parsing (casting from String to Timestamp) but instead they ignore the trailing content

To Reproduce
This test case should pass in arrow-cast/src/cast.rs

    #[test]
    fn test_cast_string_to_timestamp_invalid_tz() {
        // content after Z should be ignored
        let bad_timestamp = "2023-12-05T21:58:10.45ZZTOP";
        let array = StringArray::from(vec![Some(bad_timestamp)]);

        let data_types = [
            DataType::Timestamp(TimeUnit::Second, None),
            DataType::Timestamp(TimeUnit::Millisecond, None),
            DataType::Timestamp(TimeUnit::Microsecond, None),
            DataType::Timestamp(TimeUnit::Nanosecond, None),
        ];
        for dt in data_types {
            assert_eq!(
                cast(&array, &dt).unwrap_err().to_string(),
                "Invalid timezone: ZZTOP"
            );
        }
    }

Expected behavior
Test case should pass, instead it fails as the timestamp is correctly parsed

Additional context
Found upstream in apache/datafusion#8452

[alamb]

I think this will be a nice first issue as it has a test already written and should be a simple fix

[razeghi71]

Can I give this a shot?

[razeghi71]

@alamb the current behavior for cast is that it returns null for the elements that it is unable to parse. This way if we pass it a good timestamp and a bad one, it will return the parsed timestamp for good one and null for the bad one.

In the test case you provided, you are expecting an error result. Is that the behavior we want to change to? if yes what would happen to the case when we pass a good and a bad element to cast? again return error result?

[alamb]

@alamb the previous behavior for cast was that when it returned null for the elements that it was unable to parse. this way if we pass it a good timestamp and a bad one, it will returned the parsed timestamp for good one and null for the bad one.

In the test case you provided, you are expecting an error result. is that the behavior we want to change to? if yes what would happen to the case when we pass a good and a bad element to cast? again return error result?

Sorry, you are correct. I expect an error when CastOptions::safe is false. My test case should have called cast_with_options

[razeghi71]

We need this to get released to fix the issue linked to it here: apache/datafusion#8452. Is there a documentation on how the release process can be done in order to use it in datafusion?

[tustvold]

I anticipate cutting the next release in the coming weeks