-
Notifications
You must be signed in to change notification settings - Fork 11
Enable system database by default for queries #435
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
base: main
Are you sure you want to change the base?
Conversation
…, partitions, etc)
Reviewer's GuideEnables inclusion of ClickHouse system databases by default in various listing commands while adding a flexible pattern-based way to exclude databases, wiring the new filter through internal query builders and CLI options for parts, detached parts, partitions, and databases/tables. Sequence diagram for CLI partition listing with exclude_database_patternsequenceDiagram
actor User
participant CLI_partition_group
participant CLI_get_partitions
participant Internal_part as Internal_part_module
participant ClickHouse as ClickHouse_server
User->>CLI_partition_group: chadmin partition-group --exclude-database pattern
CLI_partition_group->>CLI_get_partitions: get_partitions(exclude_database_pattern=pattern)
CLI_get_partitions->>CLI_get_partitions: Render SQL template
CLI_get_partitions->>Internal_part: list_parts(ctx, database=None, exclude_database_pattern=pattern, ...)
Internal_part->>Internal_part: Build query with
Internal_part->>Internal_part: WHERE database NOT IN (information_schema, INFORMATION_SCHEMA)
Internal_part->>Internal_part: AND database NOT format_str_match(pattern)
Internal_part->>ClickHouse: Execute filtered listing query
ClickHouse-->>Internal_part: Result rows
Internal_part-->>CLI_get_partitions: Filtered parts list
CLI_get_partitions-->>CLI_partition_group: Aggregated partition groups
CLI_partition_group-->>User: Display partitions excluding pattern-matched databases
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- In
internal/database.list_databasestheexclude_databaseargument now goes throughformat_str_match, effectively changing it from an exact match to a pattern match; if this is intentional, consider renaming it toexclude_database_pattern(and/or updating any user-facing help) to avoid confusing callers that expect exact semantics. - The new
exclude_database_patternoption is added to several CLIs and internals; it would be good to double-check that its behavior is clearly differentiated from existingdatabase/database_patternfilters (e.g., how they combine) and that help strings consistently describe that it accepts LIKE-style patterns and comma-separated lists.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `internal/database.list_databases` the `exclude_database` argument now goes through `format_str_match`, effectively changing it from an exact match to a pattern match; if this is intentional, consider renaming it to `exclude_database_pattern` (and/or updating any user-facing help) to avoid confusing callers that expect exact semantics.
- The new `exclude_database_pattern` option is added to several CLIs and internals; it would be good to double-check that its behavior is clearly differentiated from existing `database`/`database_pattern` filters (e.g., how they combine) and that help strings consistently describe that it accepts LIKE-style patterns and comma-separated lists.
## Individual Comments
### Comment 1
<location> `ch_tools/chadmin/internal/database.py:69` </location>
<code_context>
- WHERE database NOT IN ('system', 'information_schema', 'INFORMATION_SCHEMA')
+ WHERE database NOT IN ('information_schema', 'INFORMATION_SCHEMA')
{% endif %}
{% if exclude_database %}
- AND database != '{{ exclude_database }}'
+ AND database NOT {{ format_str_match(exclude_database) }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing `exclude_database` from a strict inequality to a pattern-based `NOT` may widen the exclusion scope.
Previously this was an exact inequality:
```sql
AND database != '{{ exclude_database }}'
```
Now it uses pattern-based matching:
```sql
AND database NOT {{ format_str_match(exclude_database) }}
```
If `format_str_match` allows wildcards or comma-separated lists, callers expecting a single exact DB exclusion might now unintentionally exclude additional databases (e.g., `foo` also matching `foo_backup`). Please confirm that all current callers either expect this broader behavior or are updated accordingly, especially if the goal is to align with `exclude_database_pattern` semantics elsewhere.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Enable system database by default for some select queries (listing databases, tables, partitions. parts, etc)
Summary by Sourcery
Enable system database visibility in admin listing commands while adding flexible database exclusion filters.
New Features:
Enhancements: