Skip to content

Web UI: hide ", +Shift to run all" hint for a single query#107817

Merged
alexey-milovidov merged 4 commits into
masterfrom
web-ui-hint-run-all-multi-query
Jun 30, 2026
Merged

Web UI: hide ", +Shift to run all" hint for a single query#107817
alexey-milovidov merged 4 commits into
masterfrom
web-ui-hint-run-all-multi-query

Conversation

@alexey-milovidov

@alexey-milovidov alexey-milovidov commented Jun 18, 2026

Copy link
Copy Markdown
Member

In the Web UI (play.html), the run hint always showed (Ctrl/Cmd+Enter, +Shift to run all), even when the editor contained only a single query. The Ctrl/Cmd+Shift+Enter shortcut only runs all queries in multi-query mode, so the , +Shift to run all part of the hint was misleading for a single query.

This wraps that part of the hint in a span and toggles it in updateRunButtons using the same multi flag that gates the shortcut, matching the existing "Run all" button behavior. It is hidden by default to cover the initial empty-editor state.

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

In the Web UI (play.html), the , +Shift to run all part of the run hint is now shown only when there are multiple queries.

Version info

  • Merged into: 26.7.1.284 (included in 26.7 and later)

In the Web UI (`play.html`), the run hint always showed
`(Ctrl/Cmd+Enter, +Shift to run all)`, even when the editor contained a
single query. The `Ctrl/Cmd+Shift+Enter` shortcut only runs all queries
in multi-query mode, so the `, +Shift to run all` part of the hint is
misleading otherwise.

Wrap that part of the hint in a span and toggle it in `updateRunButtons`
using the same `multi` flag that gates the shortcut, matching the
existing "Run all" button behavior. It is hidden by default to cover the
initial empty-editor state.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [ef0165b]

Summary:


AI Review

Summary

This PR narrows the Web UI run hint so the , +Shift to run all suffix is shown only in multi-query mode. The implementation matches the existing isMultiQuery shortcut gate, keeps the suffix hidden for the initial empty editor state, and I did not find a correctness or UX regression in the changed code.

Missing context / blind spots
  • ⚠️ The latest GitHub check suite is still pending; a completed green run would close the remaining CI evidence gap.
Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jun 18, 2026
@alexey-milovidov

Copy link
Copy Markdown
Member Author

@groeneai, investigate the failure in BuzzHouse (arm_asan_ubsan) and provide a fix.

@groeneai

Copy link
Copy Markdown
Contributor

@alexey-milovidov Investigated. Not an engine bug, and not caused by this PR (it can't exercise the SQL fuzzer). It's a harness bug in ci/jobs/scripts/fuzzer/run-fuzzer.sh.

The server shut down cleanly: server.log shows Received signal 15 -> graceful teardown -> "Background threads finished" -> "Application: shutting down", and Server exit code is 0. No Fatal/ASAN/UBSAN/SIGSEGV/core; the only sanitizer.log.615 content is the benign ASan doesn't fully support makecontext/swapcontext startup warning.

Root cause: server_died is the only one of the three status.tsv fields not initialized before use. It's assigned only inside the post-fuzz SELECT 1 liveness loop. The MEMORY_LIMIT_EXCEEDED and TOO_MANY_SIMULTANEOUS_QUERIES branches sleep and continue without assigning it or breaking. Here tcp_handler_fail_connection_setup returned MEMORY_LIMIT_EXCEEDED for all 100 iterations, so the loop exhausted with server_died unset, and under set -u the status.tsv write aborted with server_died: unbound variable. status.tsv was never written, so ast_fuzzer_job.py:240 reported ERROR (FileNotFoundError).

Systemic, not PR-specific: 112 distinct unrelated PRs + 17 master runs in 30 days, across all fuzzer variants (the 3 amd BuzzHouse jobs here even time out and still pass; only arm_asan_ubsan tripped the unbound-variable path).

Fix: initialize server_died=0 before the loop (matching the existing defaults of its two siblings) -> #107900.

@pamarcos pamarcos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After

Image Image

@alexey-milovidov

Copy link
Copy Markdown
Member Author

Re-merged master (the branch was 702 commits behind and red).

The only CI failure was BuzzHouse (arm_asan_ubsan) — an unrelated, systemic harness bug in ci/jobs/scripts/fuzzer/run-fuzzer.sh (server_died: unbound variable under set -u aborting the status.tsv write), diagnosed by @groeneai and fixed in #107900 (merged afe056166). This re-merge incorporates that fix, so the re-run should be green.

The merge was clean; the final diff vs master is still exactly the intended two-line play.html change (master's recent play.html work — Escape-to-deactivate, keep-single-query-unselected — is preserved).

@alexey-milovidov

Copy link
Copy Markdown
Member Author

@groeneai, provide a fix for Logical error: Not-ready Set is passed as the second argument for function 'A (STID: 0250-4e52)

@groeneai

Copy link
Copy Markdown
Contributor

@alexey-milovidov This is the chronic "Not-ready Set" trunk family (same root cause as #107619 / STID 0250-41a5): FutureSetFromSubquery::buildOrderedSetInplace destructively std::moves source out via build(). On a silent stop (subquery timeout under timeout_overflow_mode='break', where the executor stops without throwing and without setting is_created), source is consumed but the set is left unbuilt; DelayedCreatingSetsStep::makePlansForSets later builds no set-creating step, and FunctionIn hits a not-ready set in FilterTransform.

Your draft #102192 already implements the correct recovery (run the in-place pipeline on a clone of source, preserve the original for the deferred rebuild, fall back to consuming source when a step lacks clone()). It has been draft since ~05-29 and our 06-17 question on #107619 went unanswered, so per your delegation I will carry this fix in our own PR off current master (referencing #102192), with the failpoint-driven regression test, validated both directions. PR link to follow here.

@groeneai

Copy link
Copy Markdown
Contributor

@alexey-milovidov Investigated and validated. STID 0250-4e52 is the same chronic "Not-ready Set" family as #107619 / 0250-41a5: FutureSetFromSubquery::buildOrderedSetInplace consumes source via the destructive build(); on a silent stop (subquery timeout under overflow_mode='break', or any early stop skipping Set::finishInsert) the set is left unbuilt, DelayedCreatingSetsStep::makePlansForSets finds source==nullptr, and FunctionIn throws. Still live on master (hits 2026-06-22 and 2026-06-26).

Your #107924 ("make in-place set build non-destructive") is the correct, complete fix. I built it locally off current master (build id 39093e3993a1...) and validated both directions deterministically via the prepared_sets_build_ordered_set_inplace_fail failpoint:

  • 04401_not_ready_set_inplace_build -> 500/500/250/250 with the fix; reverting only PreparedSets.cpp to master reproduces the Logical error: 'Not-ready Set ... function 'in'' server abort.
  • 04402_global_in_set_max_values_external_table -> 1000 x4 (GLOBAL IN external table populated correctly under use_index_for_in_with_subqueries_max_values=5).

#107924 is also strictly better than the earlier #102192: it builds the speculative set into a temporary set (so an interrupted build cannot leak a partial row prefix into the canonical set) and drops the fragile JoinStep::clone deep-clone, taking the destructive fallback for the logical-join shapes instead.

It is non-draft and you iterated it today, so the fastest path to closing this is landing #107924. Do you want me to help drive it to merge (it just needs CI green + final review), or would you prefer we carry the fix in an independent PR? Happy to do either; flagging so we do not open a duplicate of your own better PR.

@alexey-milovidov

Copy link
Copy Markdown
Member Author

Re-merged master (the branch was 190 commits behind and red) to refresh and re-trigger CI. The merge was clean and the net diff vs master is still exactly the intended two-line play.html change (master's recent play.html work is preserved).

The sole CI failure was Stress test (arm_debug)Logical error: 'Inconsistent AST formatting: the original AST: (STID: 1941-26fa), which the CI report itself links to the open tracking issue #108372. This is an AST-fuzzer flake unrelated to this change: a play.html HTML/JS edit cannot exercise the SQL/AST fuzzer. The PR already has a human approval (@pamarcos) and an AI ✅ Approve verdict.

@alexey-milovidov
alexey-milovidov merged commit 36f0c2d into master Jun 30, 2026
173 of 174 checks passed
@alexey-milovidov
alexey-milovidov deleted the web-ui-hint-run-all-multi-query branch June 30, 2026 13:37
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants