Skip to content

fix(lakehouse): allow append task without append row flags#19

Open
francoischalifour wants to merge 3 commits into
mainfrom
fc/improve-critical-test-coverage
Open

fix(lakehouse): allow append task without append row flags#19
francoischalifour wants to merge 3 commits into
mainfrom
fc/improve-critical-test-coverage

Conversation

@francoischalifour

Copy link
Copy Markdown
Member

The append command has two public forms:

altertable append --catalog db --schema public --table events --data '{"id":1}'
altertable append task <task-id>

The second form is documented and listed in the command help, but the command group previously reused the required append-run arguments at the parent level. That meant altertable append task <task-id> could be blocked by missing run-only flags such as --catalog, --schema, --table, and --data before the CLI reached the task subcommand.

While improving tests, also found some lakehouse tests were coupled to implementation details like operation plans and request-builder return objects. Those tests were refactored toward command-level behavior so they better survive internal refactors.

Proposed Solution

This branch changes append command wiring so the parent command keeps optional versions of the append-run flags, while the run leaf still owns the required validation. This preserves the shorthand default form:

altertable append --catalog memory --schema main --table users --data '{"id":1}'

and allows the documented task form to work independently:

altertable append task 11111111-2222-3333-4444-555555555555

The lakehouse tests now exercise public CLI behavior through the command runtime and mock HTTP logging instead of calling lower-level request builders directly.

A new shell-level test, tests/lakehouse_test.sh, covers:

  • append defaulting to the run command.
  • append task <task-id> fetching task status without requiring append-run flags.

The new shell test is included in the standard ./scripts/verify.sh offline test loop.

@leonkenneth

Copy link
Copy Markdown
Contributor

I had to dig the codebase to know about /tasks/uuid as I couldn't find it in the docs. Without the code and just looking at the command I wouldn't have known what it is for.

It may be just me, but I just don't see the use-case in the CLI, or at least I wouldn't suspect this form?

@francoischalifour

Copy link
Copy Markdown
Member Author

I was aiming for OpenAPI parity — thought it would make sense to get a task status from the command-line. I haven't played with it much so no strong feeling whether it should be promoted in the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants