Repository navigation
fix(tool/list-tables): trim whitespace in postgres/cockroachdb table_… - #4306
beskvladimir-create wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SQL queries for listing tables in CockroachDB and PostgreSQL to handle surrounding whitespace in comma-separated table names, and adds a corresponding integration test. The reviewer suggested a more efficient and idiomatic approach using regexp_split_to_array to split and trim the table names in a single operation, avoiding the overhead of a subquery with unnest and btrim.
|
@googlebot I signed it! |
…names filter
postgres-list-tables and cockroachdb-list-tables split table_names on comma
via string_to_array without trimming, so a value like "t1, t2" became
{'t1',' t2'} and the second element never matched, silently omitting the
table. Trim each split element before matching, consistent with the
mssql and spanner list-tables tools.
Fixes googleapis#4305
2c58512 to
3ee8e52
Compare
…es table_names Addresses review feedback on the postgres list-tables filter: replace the unnest + btrim subquery with a single regexp_split_to_array(TRIM($1), '\s*,\s*') array operation, which is more idiomatic and also trims any surrounding whitespace, not only spaces. CockroachDB keeps string_to_array + btrim.
…achdb table_names Apply the same single-array split to the CockroachDB query as the Postgres one. regexp_split_to_array is a registered CockroachDB builtin, so this drops the unnest + btrim subquery and trims all surrounding whitespace.
|
Hi @beskvladimir-create Thank you for your contributions. |
Description
postgres-list-tablesandcockroachdb-list-tablessplittable_nameson comma viastring_to_arraywithout trimming, so a value like "t1, t2" became {'t1', ' t2'} and the second element never matched. The table was silently omitted with no error, which to an agent reads as "the table doesn't exist". This trims each split element before matching, consistent withmssql-list-tables(LTRIM/RTRIM) andspanner-list-tables(regexp split on \s*,\s* plus TRIM).Verified against PostgreSQL 16: with the fix, "t1, t2" and messy input such as " t1 , t2 " both return all named tables, and the empty-string no-filter guard is unchanged. Added an integration test case to the shared Postgres list-tables test (runs for postgres, alloydb, cloudsql-pg). CockroachDB has no list-tables integration test today, so its identical fix is covered by review.
Security: no change to parameter binding. table_names is still passed as the bound $1 parameter; this only changes how the already-bound value is split and compared, so there is no new SQL/query-injection surface.
PR Checklist
Opened an issue first: yes (#4305). Reviewed the full diff: yes. Tests and linter: to be validated by CI. Code coverage: a test was added. Docs: not necessary. Breaking change: no.
Issue Reference
Fixes #4305 🦕