-
Notifications
You must be signed in to change notification settings - Fork 2
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
DMS/DynamoDB/MongoDB: Use SQL with parameters instead of inlining values #35
Conversation
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.
I'm not able to test but based in the logic I think this looks good.
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 otherwise. Thx!
Manage details about a SQL parameterized clause, including column names, parameter names, and values. | ||
""" | ||
|
||
columns: t.List[str] = Factory(list) |
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.
Is this used anywhere? Looks to me this is only set but never read, but maybe I missed something.
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.
Thanks for your positive review. Within this iteration, only the body of record values have been parameterized, i.e. the SET clause
. The parameterization of values within the WHERE
clauses have not been tackled yet.
@wierdvanderhaar: Software tests will be conducted on behalf of CrateDB Toolkit, but commons-codec will also receive an integration test suite in the future.
@property | ||
def set_clause(self): | ||
""" | ||
Render a SET clause of an SQL statement. | ||
""" | ||
return ", ".join([f"{key}={value}" for key, value in zip(self.columns, self.names)]) |
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.
Thanks for looking at the details. self.columns
is used right here to render the parameterized SET
clause.
|
||
columns: t.List[str] = Factory(list) | ||
names: t.List[str] = Factory(list) | ||
values: t.List[str] = Factory(list) |
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.
Most probably though, self.values
is not used yet. However, there are also other shortcomings to be addressed on a subsequent iteration, see my followup PR comment.
- `SQLOperation` bundles data about an SQL operation, including statement and parameters. - `DynamoDBFullLoadTranslator` supports full-load data transfers.
About
At the core of this patch is the introduction of the
SQLOperation
class, which bundles together an SQL statement and its parameters, in order to convey those instances to callers, instead of rendering a full-fledged SQL statement in string format, which is problematic because of string escaping issues.commons-codec/src/commons_codec/model.py
Lines 87 to 96 in 3f9dfe5
Details
SQLOperation
bundles data about an SQL operation, including statement and parameters.DynamoDBFullLoadTranslator
supports full-load data transfers.References
This patch will need relevant adjustments in CrateDB Toolkit, effectively converging into releases of both packages in lock-step.
/cc @surister, @hammerhead, @hlcianfagna