korbit-ai[bot] commented on code in PR #35936:
URL: https://github.com/apache/superset/pull/35936#discussion_r2482493765
##########
superset/utils/urls.py:
##########
@@ -47,15 +47,20 @@ def modify_url_query(url: str, **kwargs: Any) -> str:
Replace or add parameters to a URL.
"""
parts = list(urllib.parse.urlsplit(url))
- params = urllib.parse.parse_qs(parts[3])
+ pairs = urllib.parse.parse_qsl(parts[3],keep_blank_values=True)
for k, v in kwargs.items():
- if not isinstance(v, list):
- v = [v]
- params[k] = v
-
- parts[3] = "&".join(
- f"{k}={urllib.parse.quote(str(v[0]))}" for k, v in params.items()
- )
+ idxs = [i for i,(kk,_) in enumerate(pairs) if kk==k]
+ if idxs:
+ pos = min(idxs)
+ pairs = [(kk,vv) for (kk,vv) in pairs if kk!=k]
+ new=[(k,str(x)) for x in v] if isinstance(v, (list,tuple)) else
[(k,str(v))]
+ pairs[pos:pos]=new
Review Comment:
### Expensive list slice insertion operation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using slice assignment to insert elements at a specific position in a list
has O(n) time complexity due to the need to shift all subsequent elements.
###### Why this matters
This operation becomes expensive for large query strings as it requires
moving all elements after the insertion point, contributing to the overall poor
performance of the function.
###### Suggested change ā *Feature Preview*
Build the result list incrementally without slice insertions, or use a more
efficient data structure like collections.deque for insertions, then convert
back to list at the end.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c63599d-775f-443f-a86b-03c92f5335e1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c63599d-775f-443f-a86b-03c92f5335e1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c63599d-775f-443f-a86b-03c92f5335e1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c63599d-775f-443f-a86b-03c92f5335e1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c63599d-775f-443f-a86b-03c92f5335e1)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bc280d8c-54af-445d-8931-8b77bc0d11a4 -->
[](bc280d8c-54af-445d-8931-8b77bc0d11a4)
##########
superset/utils/urls.py:
##########
@@ -47,15 +47,20 @@ def modify_url_query(url: str, **kwargs: Any) -> str:
Replace or add parameters to a URL.
"""
parts = list(urllib.parse.urlsplit(url))
- params = urllib.parse.parse_qs(parts[3])
+ pairs = urllib.parse.parse_qsl(parts[3],keep_blank_values=True)
for k, v in kwargs.items():
- if not isinstance(v, list):
- v = [v]
- params[k] = v
-
- parts[3] = "&".join(
- f"{k}={urllib.parse.quote(str(v[0]))}" for k, v in params.items()
- )
+ idxs = [i for i,(kk,_) in enumerate(pairs) if kk==k]
+ if idxs:
+ pos = min(idxs)
+ pairs = [(kk,vv) for (kk,vv) in pairs if kk!=k]
Review Comment:
### Inefficient double iteration over query parameters <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code performs two separate iterations over the pairs list - first to
find indices, then to filter out matching keys, resulting in O(nĀ²) time
complexity when updating multiple parameters.
###### Why this matters
This approach scales poorly with large query strings or when updating many
parameters, as each parameter update requires scanning the entire pairs list
twice.
###### Suggested change ā *Feature Preview*
Use a single pass approach to build the new pairs list while tracking
positions, or use a dictionary-based approach for better performance:
```python
result_pairs = []
for i, (kk, vv) in enumerate(pairs):
if kk not in kwargs:
result_pairs.append((kk, vv))
elif kk in kwargs and kk not in processed:
# Insert new values at first occurrence position
v = kwargs[kk]
new = [(kk, str(x)) for x in v] if isinstance(v, (list, tuple)) else
[(kk, str(v))]
result_pairs.extend(new)
processed.add(kk)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/904d0183-767d-4d6b-80cf-207213682ca8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/904d0183-767d-4d6b-80cf-207213682ca8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/904d0183-767d-4d6b-80cf-207213682ca8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/904d0183-767d-4d6b-80cf-207213682ca8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/904d0183-767d-4d6b-80cf-207213682ca8)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ff6f76e2-cd5a-4fc5-aabf-5a482d974f02 -->
[](ff6f76e2-cd5a-4fc5-aabf-5a482d974f02)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
