Fix Register converter for DataType.BIT - #828
Conversation
mfussenegger
left a comment
There was a problem hiding this comment.
Left two suggestions - otherwise lgtm
| >>> cursor = connection.cursor(converter=DefaultTypeConverter()) | ||
|
|
||
| >>> connection.client.set_next_response({ | ||
| ... "col_types": [25], | ||
| ... "rows":[ [ "B'0110'" ] ], | ||
| ... "cols":[ "flags" ], | ||
| ... "rowcount":1, | ||
| ... "duration":1 | ||
| ... }) | ||
|
|
||
| >>> cursor.execute('') | ||
|
|
||
| >>> cursor.fetchone() | ||
| ['0110'] |
There was a problem hiding this comment.
This looks rather odd as documentation - especially the cursor.execute('') will likely be rather confusing. Can we hide the set_next_response and have a cursor.execute("select b'0110'") instead?
There was a problem hiding this comment.
Thanks, you are right. I changed it to cursor.execute("select b'0110'"). I didn't understand what you meant "hide the set_next_response"
There was a problem hiding this comment.
hide: directives like we use in the CrateDB docs. E.g.:
This way it won't show up in the rendered documentation.
| ConverterFunction = Callable[[Optional[Any]], Optional[Any]] | ||
| ColTypesDefinition = Union[int, List[Union[int, "ColTypesDefinition"]]] | ||
|
|
||
| _BIT_LITERAL = re.compile(r"B'([01]*)'") |
There was a problem hiding this comment.
| _BIT_LITERAL = re.compile(r"B'([01]*)'") | |
| _BIT_LITERAL = re.compile(r"B'([01]+)'") |
I think empty bitstrings are not possible? But please double check
There was a problem hiding this comment.
hey, I checked, they work:
>>> SELECT B''
{"cols":["B''"],"col_types":[25],"rows":[["B''"]],"rowcount":1}
>>> SELECT ''::bit(0)
{"cols":["CAST('' AS bit(0))"],"col_types":[25],"rows":[["B''"]],"rowcount":1}
>>> SELECT B'0110'::bit(0)
{"cols":["CAST(B'0110' AS bit(0))"],"col_types":[25],"rows":[["B''"]],"rowcount":1}
>>> CREATE TABLE tabel (b bit(0))
{"cols":[],"col_types":[],"rows":[[]],"rowcount":1}
and unrelated to this ticket:
>>> SELECT ''::bit(1)
{"error":{"message":"StringIndexOutOfBoundsException[Index 0 out of bounds for length 0]","code":5000}}
>>> SELECT NULL::bit(4)
{"error":{"message":"NullPointerException[Cannot invoke \"io.crate.sql.tree.BitString.length()\" because \"bs\" is null]","code":5000}}
There was a problem hiding this comment.
Both will be fixed in 6.4.5 with crate/crate#20155 and crate/crate#20156
|
Tick the box to add this pull request to the merge queue (same as
|
I also reproduced, and yes, looks like a bug. I think crate's 500 codes shouldn't reach all the way here, they should report as a 4000 error. Could you create a bug report on crate/crate? |
Summary of the changes / Why this is an improvement
DataType.BIThad an enum member but no entry in_DEFAULT_CONVERTERS, soBITvalues fell through_to_defaultand reached Python in CrateDB's SQL literal form,B'0110'. That form is not accepted back as a query parameter, which made the round-trip asymmetric._to_bit_stringnow strips the literal wrapper and returns a plain string of0/1digits, so what you read is what you can write.Checklist