-
Notifications
You must be signed in to change notification settings - Fork 88
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
Use descriptors for column binding information #302
Conversation
* master: Rollback to Nullable(Nothing) for unbound params Actualize the comment Update test/deploy_and_run_clickhouse_macos.sh Fix Null parameter format Switch to ClickHouse version 20.3 as a baseline Fixing Decimal tests and removing them as known fails. Fixes to match known fails. Fixing issue with description formatting. First changes to support TestFlows 1.6
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.
Looks Ok, however, some comments are missing here and there, please fix it at it would be Ok to merge.
SQLBindCol( | ||
hstmt, | ||
1, | ||
getCTypeFor<std::decay_t<decltype(col1[0])>>(), |
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.
This looks somewhat error prone here, why not just getCTypeFor(col1[0])
?
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.
The current getCTypeFor
definition will cause the code to fail at compilation, so it is safe. Ugly here, but safe everywhere.
driver/utils/type_info.h
Outdated
@@ -1813,6 +1813,9 @@ namespace value_manip { | |||
template <typename SourceType> | |||
struct from_value { | |||
static inline SQLRETURN convert(const SourceType & src, BindingInfo & dest) { | |||
if (dest.indicator && dest.indicator != dest.value_size) | |||
*dest.indicator = 0; // Value is not null here... |
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.
Not quite sure what indicator
means here, could you please add some comments to BindingInfo
? Please also specify what does it mean when indicator
value is 0
.
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.
Modified the comments.
Reduce row set sizes to reduce test running time
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.
Ok
Use descriptors for column binding information.
Fix column-wise vs row-wise binding for parameters and implement it for columns.
Implement array bindings for columns.
Closes #235
Closes #296
Doc modification reflects findings in #293
Doc modification reflects versioning issues mentioned in #289
Should help with #299
Should help with #295