-
Notifications
You must be signed in to change notification settings - Fork 421
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
Allow to specify arguments from the multiple tables for prepared queries #3055
Conversation
Codecov Report
Continue to review full report at Codecov.
|
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 good, I have a few minor remarks. I have mixed feelings about the new portmanteau tabcol, but I think it's ok :)
src/rdbms/mongoose_rdbms_odbc.erl
Outdated
@@ -177,11 +181,11 @@ field_name_to_mapper(_ServerType, TableDesc, FieldName) -> | |||
fun(P) -> {ODBCType, [P]} end | |||
end. | |||
|
|||
field_to_odbc_type(FieldName, TableDesc) -> | |||
case lists:keyfind(FieldName, 1, TableDesc) of | |||
field_to_odbc_type(TabCol = {Table, Column}, TableDesc) -> |
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 should be named 'tabcol_to_odbc_type' for consistency.
@@ -76,18 +78,20 @@ query(Connection, Query, Timeout) -> | |||
Fields :: [binary()], Statement :: iodata()) -> | |||
{ok, {binary(), [fun((term()) -> tuple())]}}. | |||
prepare(Connection, Name, Table, Fields, Statement) -> | |||
try prepare2(Connection, Table, Fields, Statement) | |||
TabCols = fields_to_tabcol(Fields, Table), |
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.
Just to make sure: the only reason we don't just accept TabCols
as an argument instead of Fields
is that you consider the 'tab.col'
syntax better than {tab, col}
, right? I am aware that we would need to accept both a single atom or a tuple as the argument.
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 main reason is that it's the same syntax as in SQL queries, so less confusion, easier to write such queries.
We don't do "SELECT {tab, col}", so I am not reinventing the wheel :)
So, for me it just looks more natural, even if we have to split it in the backed module.
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.
Also, it's the same reason why Fields are atoms, even if we convert them to binaries in the prepare call.
It just looks a bit cleaner.
tabcol is just because "fully specified column name" is too long. |
This PR addresses allows to specify arguments from the multiple table. Which is needed for pubsub queries.
Proposed changes include:
table.column
as a field name into theprepare
callcolumn
still works.