Skip to content

Fix Register converter for DataType.BIT - #828

Open
bgunebakan wants to merge 2 commits into
mainfrom
bilal/817-register-converter-for-datatypebit
Open

Fix Register converter for DataType.BIT#828
bgunebakan wants to merge 2 commits into
mainfrom
bilal/817-register-converter-for-datatypebit

Conversation

@bgunebakan

@bgunebakan bgunebakan commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary of the changes / Why this is an improvement

DataType.BIT had an enum member but no entry in _DEFAULT_CONVERTERS, so BIT values fell through _to_default and 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.

cur.execute("SELECT flags FROM t")            # -> "B'00000001'"
cur.execute("... WHERE flags = ?", (value,))  # ProgrammingError: Cannot cast value
_to_bit_string now strips the literal wrapper and returns a plain string of 0/1 digits

_to_bit_string now strips the literal wrapper and returns a plain string of 0/1 digits, so what you read is what you can write.

Checklist

@bgunebakan bgunebakan self-assigned this Sep 2, 2026
@bgunebakan bgunebakan linked an issue Sep 2, 2026 that may be closed by this pull request

@mfussenegger mfussenegger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two suggestions - otherwise lgtm

Comment on lines +386 to +399
>>> 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']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

ConverterFunction = Callable[[Optional[Any]], Optional[Any]]
ColTypesDefinition = Union[int, List[Union[int, "ColTypesDefinition"]]]

_BIT_LITERAL = re.compile(r"B'([01]*)'")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_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

@florinutz florinutz Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}}

@mergify

mergify Bot commented Sep 7, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@florinutz florinutz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

the 2 statements I found return http 500 with a java exception instead of a SQL error (crate 6.4.1). They are out of the scope of this ticket, and I think we should raise it as bug(s?) for crate/crate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Register converter for DataType.BIT

3 participants