New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MySQL binary protocol implementation for initial support of Tableau Online #54115
MySQL binary protocol implementation for initial support of Tableau Online #54115
Conversation
This is an automated comment for commit 44e1f6b with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
This is an automatic comment. The PR descriptions does not match the template. Please, edit it accordingly. The error is: Changelog entry required for category 'Improvement' |
4 similar comments
This is an automatic comment. The PR descriptions does not match the template. Please, edit it accordingly. The error is: Changelog entry required for category 'Improvement' |
This is an automatic comment. The PR descriptions does not match the template. Please, edit it accordingly. The error is: Changelog entry required for category 'Improvement' |
This is an automatic comment. The PR descriptions does not match the template. Please, edit it accordingly. The error is: Changelog entry required for category 'Improvement' |
This is an automatic comment. The PR descriptions does not match the template. Please, edit it accordingly. The error is: Changelog entry required for category 'Improvement' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second batch of comments.
break; | ||
} | ||
case TypeIndex::UInt16: { | ||
UInt16 value = assert_cast<const ColumnVector<UInt16> &>(*col).getData()[row_num]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are casting *col
to the specific column implementation of the corresponding type (e.g. ColumnVector<UInt16>
). That is okay.
But: This only works if col
is not of low-cardinality (ColumnLowCardinality
), or nullable (ColumnNullable), sparse (ColumnSparse) or const (ColumnConst) type. These classes are (possibly chained) wrappers around the actual column structure. In l. 164, we are stripping the low-cardinality and nullness of the datatype away, but we would need to do something equivalent for col
. Check e.g. recursiveRemoveSparse()
(Columns/ColumnSparse.h), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LowCardinality and Nullable are removed prior to that, so I assumed it should always work as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are only removed from the data type, not from the actual data. I am pretty sure this will go boom, please try it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumping the 3rd batch of comments. More tomorrow.
tests/integration/test_mysql_protocol/prepared_statements_test.sql
Outdated
Show resolved
Hide resolved
…-statements-for-mysql' into simplified-prepared-statements-for-mysql
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thanks.
Kindly look at the remaining (mostly small) comments, we can submit afterwards.
break; | ||
} | ||
case TypeIndex::UInt16: { | ||
UInt16 value = assert_cast<const ColumnVector<UInt16> &>(*col).getData()[row_num]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are only removed from the data type, not from the actual data. I am pretty sure this will go boom, please try it out.
1. Disables namespace indentation in accordance with 16. of (*) and the majority of the codebase. 2. Disables {} for single-statement if/for/while in accordance with 17. of (*) and the majority of the codebase. Note: clang-format is used manually and voluntarily, usually only on new files. Nothing is automatically reformatted, this is only about how new files look. Motivated by ClickHouse#54115 (comment). (*) https://clickhouse.com/docs/en/development/style#formatting
Is it possible to disable this feature using CH option? It introduce several protocol violations making it unusable (I will try to fill found issues if they are not reported yet) as client library now switch to binary protocol |
@MaceWindu No, it is not possible to disable the binary protocol as it an integral part of the MySQL wire protocol. If you run into problems due to this PR, kindly open an issue, ideally with a small example that doesn't work. |
Resolves #51103.
This PR allows connecting to Tableau Online, discovering the tables, and fetching the data.
Tableau Online wraps everything into prepared statements, even the simplest
SELECT
s. However, it does not add the parameters to bind - everything is hardcoded into the query. The current MySQL interface implementation was missing binary protocol implementation, as well asCOM_STMT_PREPARE
,COM_STMT_EXECUTE
, andCOM_STMT_CLOSE
handlers. This PR addresses this.Please note: the prepared statements implementation is pretty minimal, we do not support arguments binding yet, it is not required in this particular Tableau online use case. It will be implemented as a follow-up if necessary after extensive testing of Tableau Online in case we discover issues.
I also tweaked
SHOW COLUMNS
a bit as it did not 100% match the MySQL spec.Corner cases of Decimal128 with precision > 65 or scale > 30 were also addressed, as this is the maximum according to the spec.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
The MySQL interface gained a minimal implementation of prepared statements, just enough to allow a connection from Tableau Online to ClickHouse via the MySQL connector.