feat: add dremio space commands and reject single-component paths in folder create#24
feat: add dremio space commands and reject single-component paths in folder create#24sandhyasun wants to merge 7 commits into
dremio space commands and reject single-component paths in folder create#24Conversation
…n `folder create` Spaces and folders are distinct catalog concepts but the CLI blurred them: `dremio folder create "foo"` silently rewrote to CREATE SPACE, which breaks on projects without Space Plugin and hides the semantic difference. Changes: - `dremio folder create` now rejects single-component paths with a clear error pointing users to `dremio space create <name>` instead. - New `dremio space` command group with `list`, `get`, `create`, and `delete` subcommands. `space list` filters the catalog to SPACE entities; the others delegate to the existing folder helpers. Closes dremio#13 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f3f18572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
On DCS clusters without Space Plugin enabled, DCSCoordinatorCatalogServiceImpl.createSpace() rejects CREATE SPACE SQL with "Legacy spaces are not supported." In that case, fall back to CREATE FOLDER with a single-component path, which works on those clusters because the single-component validation was added alongside Space Plugin. All other failures (entity already exists, permissions, etc.) are propagated immediately without triggering the fallback. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- folder get/delete reject single-component paths (redirect to space get/delete) - space get/delete reject multi-component paths (redirect to folder get/delete) - create_folder propagates SQL job failures as DremioAPIError - create_space fallback (pre-SP clusters): check fallback result and raise on failure; rewrite opaque "Folder [[source, name]] already exists" to "Space [name] already exists" Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Prevents dremio space delete from deleting non-space entities (sources, home folders) that happen to share a single-component name. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
Still in Draft . Need to look into maintaining compatibility pre/post Space Plugin once more. |
- folder create: replace ValueError with deprecation warning + server-driven behavior (pre-SP succeeds, SP fails); update help text to match - folder get/delete: keep single-component redirect; update help text - folder group: update module docstring and app help to reflect dual purpose (nested folders + top-level catalog listing) - output.py: add warn() for stderr deprecation warnings - introspect.py: add space.list/get/create/delete entries; fix stale folder.create description and sql_template - tests: add deprecation warning, pre-SP success, and SP failure tests for folder create single-component path Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- Add dremio space to command summary table - Split CRUD table: Spaces row -> space.*, Folders row -> nested-only - Fix folder create description: uses CREATE FOLDER; single-component paths deprecated and may fail on Space-Plugin-enabled clusters - Add space.py to file tree Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
| async def create_folder(client: DremioClient, path: str) -> dict: | ||
| """Create a space (single component) or folder (nested path) using SQL.""" | ||
| """Create a folder at the given path using SQL.""" | ||
| from drs.utils import DremioAPIError |
There was a problem hiding this comment.
lets move imports to top level
| sql = f"CREATE FOLDER {quoted}" | ||
| return await run_query(client, sql) | ||
| warn( | ||
| f"Top-level folder creation is deprecated. " |
There was a problem hiding this comment.
- lets not talk about "Space-Pluging-enabled" - its DC and there are no clusters
- more importantly we should just fail this operation right away, no ?
| Nested paths (e.g. 'Analytics.reports') create a folder inside a space. | ||
| Single-component paths attempt top-level creation for compatibility with | ||
| pre-Space-Plugin clusters; this is deprecated — use `dremio space create` | ||
| instead. On Space-Plugin-enabled clusters, single-component paths will |
There was a problem hiding this comment.
lets change the comments here - this is DC so there are no clusters or configs
| async def create_space(client: DremioClient, name: str) -> dict: | ||
| """Create a space. | ||
|
|
||
| Uses CREATE SPACE SQL. On pre-Space-Plugin clusters that reject it with |
| async def get_space(client: DremioClient, name: str) -> dict: | ||
| """Get space metadata by name.""" | ||
| if len(parse_path(name)) > 1: | ||
| raise ValueError(f"'{name}' is a nested path. Use `dremio folder get {name}` for folders.") |
There was a problem hiding this comment.
Instead of all these ValueError change to an error that extends ValueError - NestedPathUnsupported. That way the function doesn't always assume the error is to be reported to the user
| except Exception as exc: | ||
| from drs.utils import DremioAPIError | ||
|
|
||
| if isinstance(exc, DremioAPIError): | ||
| error(str(exc)) | ||
| raise typer.Exit(1) | ||
| if isinstance(exc, ValueError): | ||
| error(str(exc)) | ||
| raise typer.Exit(1) | ||
| raise |
There was a problem hiding this comment.
should be replaced by
except:
error(...)
raise typer.Exit(1)
| try: | ||
| return await coro | ||
| finally: | ||
| await client.close() |
There was a problem hiding this comment.
shouldn't the caller worry about client.close() ... the caller has created the client, we shouldn't close it here
| def _get_client() -> DremioClient: | ||
| from drs.cli import get_client | ||
|
|
||
| return get_client() |
There was a problem hiding this comment.
the wrapper doesn't really help. Do
from contextlib import asynccontextmanager
@asynccontextmanager
async def managed_client():
client = await get_client()
try:
yield client
finally:
await client.close()
then _run_command can be removed. See below
| ) -> None: | ||
| """Get metadata for a space by name.""" | ||
| client = _get_client() | ||
| _run_command(get_space(client, name), client, fmt, fields=fields) |
There was a problem hiding this comment.
change this and otehrs below to
async with _get_client() as client:
result = asyncio.run(client, name)
output(result, fmt, fields=fields)
Summary
Fixes #13
Adds a dedicated `dremio space` command group (list / get / create / delete) and clarifies top-level vs nested catalog semantics in `dremio folder`.
New `dremio space` commands
`dremio folder` behavior changes
Net effect
Test plan