fix: casting when data to be written does not match table schema

[Blajda]

Description

Suppose a user has a table with column of type int. A user can create a record batch with type Uft8 and write the value to table. My expectation is that either the writer returns an error or ansi sql behavior is implemented where non-numeric strings are turned into nulls.

[Blajda]

Hi @wjones127,
I discovered the following while implementing the update operation.
I have a test where a user attempts to update a column with a type that does not match the table schema. I thought the writer would return an error but it wrote the data as is to the table. It will then 'corrupt' the table since further operations on it using Datafusion will error since the datatypes are incompatible.

Seems like this breaks the invariant test since the expected underlying type for integer is i32 but the array provided is u32. Hence I think we should update get_record_batch to provide an i32 array.

Let me know what you think

[rtyler]

Ignore the integration test failure 😒

final link failed: No space left on device

These GitHub Actions caches are...annoying

[rtyler]

I'm confused, where does this value go in the expected table? 😕

[wjones127]

Right below, in line 570?

[Blajda]

Since it cannot be parsed as an int it will result in a null value. Which aligns with ansi sql but maybe we should only allow that if the user opts in?

[wjones127]

Oh nevermind. It seems like this is being inserted as null? That doesn't seem like what a user would want.

[wjones127]

Yeah I think we could disable that by passing safe: true in CastOptions: https://docs.rs/arrow-cast/40.0.0/arrow_cast/cast/struct.CastOptions.html

[rtyler]

I think this is a good refactor 👍

[wjones127]

I can't find an arrow function that does more strict casting, but I kind of would prefer if we were more strict with the casting. For example, these casts aren't lossy:

• Dictionary<int8, string> -> string
• int32 -> int64

But I think we should reject these by default

• string -> int32
• timestamp -> string

[wjones127]

I think for now, I'd be fine with casting if we added safe: true by default. If you want to add an option to allow unsafe casting to null, I think that's fine.

[Blajda]

The name is confusing but the default behavior we want is safe: false which will return an error. safe: true will write a null value.
I expanded the test to include the timestamp case however it does write the timestamp in the RFC format without issue. Should we accept this?

[wjones127]

Should we accept this?

I think this is an improvement.

If we want to be more strict, we should write a function that checks the schema the user passed and compare it to the table, returning an error if it doesn't logically match. But I think that's fine to do as a follow up.