Include the column name in the error message for an unexpected NULL #397
Conversation
clickhouse_connect/driver/insert.py
Outdated
| @@ -198,3 +199,12 @@ def _convert_numpy(self, np_array): | |||
| data[ix] = data[ix].tolist() | |||
| self.column_oriented = True | |||
| return data | |||
|
|
|||
| def start_column(self, name: str): | |||
There was a problem hiding this comment.
This gets called during the insert to tell us what the current column being inserted is, so I store its name here so we have it if we run into an error during inserting that column
clickhouse_connect/driver/insert.py
Outdated
| self._column_name = name | ||
|
|
||
| def make_data_error(self, error_message: str) -> DataError: | ||
| if self._column_name is not None: |
There was a problem hiding this comment.
Here's where we use the column name that was stored by start_column(). I don't know if it's possible to reach here by doing a column insert without start_column() having been called, but I handled None just in case
There was a problem hiding this comment.
It's not possible, so it's safe to remove the None check.
| @@ -54,8 +55,8 @@ def write_array(code: str, column: Sequence, dest: MutableSequence): | |||
| buff = struct.Struct(f'<{len(column)}{code}') | |||
| dest += buff.pack(*column) | |||
| except (TypeError, OverflowError, struct.error) as ex: | |||
| raise DataError('Unable to create Python array. This is usually caused by trying to insert None ' + | |||
| 'values into a ClickHouse column that is not Nullable') from ex | |||
| raise ctx.make_data_error('Unable to create Python array. This is usually caused by trying to insert None ' + | |||
There was a problem hiding this comment.
This was the only error I really needed improving, but I modified the other places where DataError could be raised too, in string.py
There was a problem hiding this comment.
Thanks so much, I agree it's hard to track down obscure insert errors and this will help. Stashing the column name in InsertContext makes a lot of sense, btw -- I haven't looked deeply, but maybe we add that to the BaseQueryContext instead since it doesn't hurt to have it around for queries as well.
clickhouse_connect/driver/common.py
Outdated
| @@ -38,12 +38,13 @@ def array_type(size: int, signed: bool): | |||
| return code if signed else code.upper() | |||
|
|
|||
|
|
|||
| def write_array(code: str, column: Sequence, dest: MutableSequence): | |||
| def write_array(code: str, column: Sequence, dest: MutableSequence, ctx): | |||
There was a problem hiding this comment.
I think you can add the ctx Type here too?
clickhouse_connect/driver/insert.py
Outdated
| self._column_name = name | ||
|
|
||
| def make_data_error(self, error_message: str) -> DataError: | ||
| if self._column_name is not None: |
There was a problem hiding this comment.
It's not possible, so it's safe to remove the None check.
|
|
Sounds like a good idea, I've moved it to BaseQueryContext as you suggest. Should be ready for another CI run now |
|
Thanks again! I'm hoping to do another release next week that will include this. |
|
No problem! Great to hear, thanks |
Previously if you inserted a NULL into a column that isn't Nullable, the error you got was
which is unhelpful for working out which column is the problem. I've modified that error path so it can include the column name in the error, like so:
I felt like this was a pretty small change so I didn't add tests or file an issue, I hope that's okay.