[IMP] util.explode_query{,_range}: only format the {parallel_filter} placeholder#339
[IMP] util.explode_query{,_range}: only format the {parallel_filter} placeholder#339KangOl wants to merge 1 commit intoodoo:masterfrom
{parallel_filter} placeholder#339Conversation
|
|
Pirols
left a comment
Pirols
left a comment
There was a problem hiding this comment.
IMO the unbalanced braces issue is the least of our problems here and we can do with a backwards compatible implementation that does not tackle that.
| ] | ||
| ) | ||
| def test_ExplodeFormatter(self, value, expected): | ||
| formatted = util.pg._ExplodeFormatter().format(value, parallel_filter="…") |
There was a problem hiding this comment.
| formatted = util.pg._ExplodeFormatter().format(value, parallel_filter="…") | |
| formatted = util.pg._ExplodeFormatter().format(value, parallel_filter="...") |
np: I can imagine someone trying to add a test case being confused by the unicode character here.
There was a problem hiding this comment.
It was copied from the commit message stolen from #142
There was a problem hiding this comment.
It did look familiar. :p
For what it's worth, I have since removed the vim abbrev rule that replaced triple dots with that one.
|
upgradeci retry with always only crm |
KangOl
force-pushed
the
master-explode-formatter-chs
branch
from
e1f7f15 to
0a2a432
Compare
src/util/pg.py
Outdated
| sep_kw = " AND " if re.search(r"\sWHERE\s", query, re.M | re.I) else " WHERE " | ||
| query += sep_kw + "{parallel_filter}" | ||
|
|
||
| fmt = _ExplodeFormatter().format |
There was a problem hiding this comment.
Why don't we put this in the global scope, right after the definition of _ExplodeFormatter? It can be reused in explode_query for example.
There was a problem hiding this comment.
Let's try. I added a fixup commit to see if the final result adds up.
There was a problem hiding this comment.
Why don't we put this in the global scope, right after the definition of
_ExplodeFormatter? It can be reused inexplode_queryfor example.
So, do I squash this commit with the _explode_format variable?
KangOl
force-pushed
the
master-explode-formatter-chs
branch
from
0a2a432 to
efc6c9e
Compare
…` placeholder The usage of `str.format` to inject the parallel filter used to explode queries is not robust to the presence of other curly braces. Examples: 1. `JSON` strings (typically to leverage their mapping capabilities): See 79f3d71, where a query had to be modified to accommodate that. 2. Hardcoded sets of curly braces: ```python >>> "UPDATE t SET c = '{usage as literal characters}' WHERE {parallel_filter}".format(parallel_filter="…") Traceback (most recent call last): File "<stdin>", line 1, in <module> KeyError: 'usage as literal characters' ``` Which can be (unelegantly) solved adding even more braces, leveraging one side effect of `str.format`: ```python >>> "UPDATE t SET c = '{{usage as literal characters}}' WHERE {parallel_filter}".format(parallel_filter="…") "UPDATE t SET c = '{usage as literal characters}' WHERE …" ``` 3. Hardcoded single unpaired curly braces (AFAICT no way to solve this): ```python >>> "UPDATE t SET c = 'this is an open curly brace = {' WHERE {parallel_filter}".format(parallel_filter="…") Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: unexpected '{' in field name ``` ```python >>> "UPDATE t SET c = 'this is a close brace = }' WHERE {parallel_filter}".format(parallel_filter="…") Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: Single '}' encountered in format string ``` To circumvent this, we now use a dedicated Formatter that only handle the `{parallel_filter}` placeholder. This has the advantage of still deduplicate the doubled curly braces (see point 2 above) and thus being retro-compatible. This doesn't solve the single unpaired curly braces case, but this is rare enough to be handled by other means.
KangOl
force-pushed
the
master-explode-formatter-chs
branch
from
efc6c9e to
4eacbc8
Compare
|
@robodoo r+ |
4 participants
The usage of
str.formatto inject the parallel filter used to explode queries is not robust to the presence of other curly braces. Examples:JSONstrings (typically to leverage their mapping capabilities):See 79f3d71, where a query had to be modified to accommodate that.
Which can be (unelegantly) solved adding even more braces, leveraging one side effect of
str.format:To circumvent this, we now use a dedicated Formatter that only handle the
{parallel_filter}placeholder. This has the advantage of still deduplicate the doubled curly braces (see point 2 above) and thus being retro-compatible.This doesn't solve the single unpaired curly braces case, but this is rare enough to be handled by other means.
Alternative to #142