[pull] master from cube-js:master - #707
Merged
Merged
Conversation
* fix(cubesql): stop reporting `Continue wait` as a SQL API error A `Continue wait` coming back from a parallelized plan was logged as a `Cube SQL Error` load event, so Query History showed the queue's "not finished yet" signal as a failed request. #10649 suppressed that event, but only for an error whose message is exactly `Continue wait`. DataFusion's `RepartitionExec` has to hand one error to every output partition and a boxed error is not `Clone`, so `wait_for_task` flattens ours to its `Display` string and re-wraps it as `DataFusionError::Execution`: the typed `CubeError` is gone and the message reads `Execution error: Continue wait`, which an equality check does not match. `target_partitions` defaults to `num_cpus::get()` with `repartition_aggregations` / `repartition_windows` on, so any GROUP BY or window function over a `CubeScan` takes that path. - add `CubeError::is_continue_wait()`, which matches the cause first and falls back to a substring test on the message, and use it at the call sites that each spelled the check differently - exact equality in `handle_sql_query` and `load_data`, substring in `NodeBridgeTransport` - restore the `ContinueWait` cause and the canonical message in the DataFusion and Arrow conversions, so the cause survives the flattening and the prefixes cannot compound through a further wrapping layer - also try `DataFusionError` when downcasting `DataFusionError::External`, which `From<ArrowError>` already did - classify the streaming path's errors by cause, as `load_data` does, rather than prefixing the message * fix(cubesql): address review on `Continue wait` detection Follow-up on the review of #11717. The message check is a tail match, not a substring test - the earlier comment and commit message said "substring", which is the looser semantics this deliberately avoids. - `normalize_continue_wait` canonicalizes the message whenever the error is a continue wait, not only when the cause was missing. An error can arrive with the cause already set and a prefixed message - `CubeScanMemoryStream` sets the cause without rewriting the message, and the `CubeError` downcast arm returns it unchanged - and JS compares the message exactly in places (`gateway.ts`'s `err.message === 'Continue wait'`). Fixing it centrally covers the `scan.rs` case too, so that call site stays as it is. - Match the tail of the *first line*, so a message that arrived over the JS bridge with a stack appended still resolves. `errorString` reads `err.error` and `err.message` before falling back to `err.stack`, so a continue wait does not reach that fallback today; this keeps the check robust if it ever does. - Compare as ASCII bytes instead of lowercasing both sides. The needle is pure ASCII and these messages can quote the whole failing query, so `to_lowercase` allocated a copy of it - plus one of the constant - on every call. Three more tests: a typed continue wait carrying a prefixed message, a message with a stack appended, and the awkward inputs the byte-tail compare has to survive (shorter than the needle, and a split landing inside a multi-byte character). * fix(cubesql): require `Continue wait` to start a word Follow-up nit from the review of #11717. The tail match had no word boundary, so a first line ending in a longer word - `discontinue wait` - was read as the queue's signal and would have been swallowed. The phrase now has to start a word. Every real prefix ends in a separator (`Execution error: `), and an unprefixed message has no preceding character at all, so both keep matching. * fix(cubesql): match `Continue wait` as a lowercased substring Reverts the narrowing this branch picked up over the last two commits - the tail match, the word boundary, and the ASCII byte compare - back to the check `NodeBridgeTransport` has used since 2024: message.to_lowercase().contains("continue wait") The message has to survive arbitrary wrapping on both sides: a prefix from whichever layer re-wrapped the error (`Execution error: `, `Database Execution Error: `, and these compound), and a suffix when it arrived over the JS bridge with a stack appended. Anything narrower has to enumerate those shapes, and the shape is what keeps changing - twice now a continue wait has reached query history as a failed request because a check was too specific about it. The cost, written into the doc comment rather than left implicit: a real failure quoting the phrase - an error naming a column `continue wait` - is read as the queue's signal and goes unreported. No such message is known, and the opposite mistake is the one with a track record. Two tests asserted the narrow behaviour and are updated: the quoting cases drop out of `a_real_failure_is_left_alone`, and the boundary test becomes `a_message_without_the_phrase_is_not_a_continue_wait`, keeping the empty, short and non-ASCII inputs that still must not match. * fix(cubesql): do not read a quoted `continue wait` as the queue's signal Review follow-up. The lowercased substring check feeds `normalize_continue_wait`, which runs on every DataFusion and Arrow conversion and *replaces* the message - so a false positive does not merely go unreported, it destroys the original error text and the caller sees a query that appears to poll forever instead of the error naming their mistake. These messages interpolate user-controlled SQL, so the realistic false positive is `No field named 'continue wait'` - and it is always quoted. Skip an occurrence immediately preceded by `'`, `"` or a backtick. That is one character of context rather than an enumeration of wrapper shapes: arbitrary prefixes and appended stacks still match, and the exception is per occurrence, so a message that quotes the phrase and also carries it unquoted is still a continue wait. The doc comment previously priced a false positive as "would go unreported", which was the cost at the original `NodeBridgeTransport` site, not here. Fixed. * fix(cubesql): record the Postgres cost of a misread `Continue wait` Review follow-up, doc and a constant - no behaviour change. The cost paragraph named `normalize_continue_wait` replacing the message but missed the other consumer the widened check newly reaches. `load_data` held an equality check until this branch: a real database error misread there is minted with the `ContinueWait` cause locally, so nothing re-classifies it on the way up - unlike a genuine continue wait, which the transport retries and never surfaces on the Postgres path - and `sql/postgres/error.rs` answers the client `SqlStatementNotYetComplete` (`03000`) instead of their failure. That also corrects the reachability note in the PR description, which argued the `ContinueWait` arm there was unreachable. The argument held for a genuine continue wait and does not cover an error misread into that cause. Also lifts the lowercased needle into a constant, so the substring test no longer allocates a copy of `CONTINUE_WAIT_MESSAGE` on every call, with a test keeping the two spellings in step. * fix(cubesql): match `Continue wait` by message structure, not substring Replaces the lowercased `contains` and its quote guard with a check that follows the structure these messages actually have. Every wrapping layer prepends its own label and a colon (`Execution error: `, `Database Execution Error: `, and these compound), and a message that came over the JS bridge can carry an appended stack, which puts the message on the first line and the frames after it. So the phrase always lands as a whole `:`- or newline-delimited part, however many layers wrapped it: message .split(['\n', ':']) .any(|part| part.trim().eq_ignore_ascii_case(CONTINUE_WAIT_MESSAGE)) That matches every wrapped shape without enumerating them - which is what the substring test bought - while a real failure that only mentions the phrase inside a larger part keeps its message. The quote guard covered just the shapes where the phrase is quoted, so `Execution error: continue wait is not a column` was still read as the queue's signal, and `normalize_continue_wait` replaces the message, meaning that text was gone before anything downstream saw it. The trade now runs the other way: a layer that appends rather than prepends (`Continue wait.`) would stop matching. None does today - the phrase is a wire constant that gets wrapped, not edited - and the doc comment says so. Splitting also drops both allocations the old check made per call: `to_lowercase` on a message that can quote the whole failing query, plus one for the constant. `CONTINUE_WAIT_MESSAGE_LOWER` and the test keeping it in step go with them. Tests: `the_phrase_is_matched_as_a_whole_part` pins the wrapped and stack-appended shapes on the predicate directly, and `a_real_failure_is_left_alone` gains the unquoted mid-part mention the quote guard let through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XFsWyd58aFEWhLT2CjuiiV * fix(cubesql): name the one wrapper that appends, and pin it with a test Review follow-up on the structural match. The doc comment justified the trade with "No layer produces those today - the phrase is a wire constant that gets wrapped, not edited", and that is not true as written: the `Rewrite` arm of this file's own `Display` renders Rewrite Error: {}. Please check logs for additional information where every other arm is `<Label>: {}`. Flattened through `Display` - which is exactly what `RepartitionExec::wait_for_task` does, the reason this fallback exists at all - a continue wait with that cause reads `Rewrite Error: Continue wait. Please check logs for additional information`, whose parts are `Rewrite Error` and ` Continue wait. Please check logs...`, and the predicate matches neither. Not reachable today, and I checked rather than assumed: every `CubeError::rewrite` call site is inside `compile/rewrite/**` with its own locally-authored message, while a continue wait originates from `transport.load` at execution time, after rewriting. So this is a false invariant in a comment, not a live bug - but that sentence is what the next person adding a cause variant or a wrapper will trust. The comment now names the exception and why it cannot be hit, and `every_cause_renders_a_matchable_continue_wait` pins both halves: it renders a canonical continue wait through every cause and asserts the predicate still matches, `Rewrite` excepted. Its inner match is exhaustive, so a new cause variant stops compiling here, and an existing arm taught to append fails the assertion - either way the next appending wrapper surfaces here instead of silently swallowing the queue's signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XFsWyd58aFEWhLT2CjuiiV * docs(cubesql): give the right reason the `Rewrite` arm is unreachable Review follow-up, comments only. The justification added in the previous commit was that `CubeError::rewrite` is minted only inside the rewrite engine with its own messages. That is not what makes the arm safe, and it is not true: `query_engine.rs` re-stamps `e.cause = CubeErrorCauseType::Rewrite(..)` onto whatever `compiler_cache.rewrite` and `find_best_plan` return, whatever its origin, and `converter.rs` already wraps a foreign `MemberError` with `CubeError::rewrite(error.to_string())`. So the cause is not confined to locally-authored messages, and a reader trusting that sentence would think the arm safe by construction. The guarantee is one level down: the rewrite phase never calls the transport. It works against an already-fetched `MetaContext` / `CompilerCacheEntry`, and `compile/rewrite/` contains no `transport.` call at all, so nothing there can produce a continue wait for `query_engine.rs` to re-label. That reason survives someone adding another foreign-error wrap; the previous one did not. Both the predicate's doc comment and the mirrored paragraph on `every_cause_renders_a_matchable_continue_wait` now say this. No behaviour or test change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XFsWyd58aFEWhLT2CjuiiV * chore(review): cap explanatory comments at three lines Adds a Comments section to the Claude review prompt. Two rules: an explanatory comment is three lines at most, and it earns its place only when its absence would let a later edit reintroduce the bug. A comment is a claim nothing recompiles, so it stops being checked the moment the code under it moves, and a long one buries the sentence that was actually load-bearing. The section also names what to prefer instead - a named constant, a named intermediate value, an extracted function - and bounds the rule so it is never raised to ask for a comment to be added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XFsWyd58aFEWhLT2CjuiiV --------- Co-authored-by: Claude <noreply@anthropic.com>
…MPILER_CACHE_SIZE` (#11732) * feat(server-core): add CUBEJS_COMPILER_CACHE_SIZE env variable `compilerCacheSize` could only be set through the configuration file. Add a `CUBEJS_COMPILER_CACHE_SIZE` environment variable that feeds the same option, so the compiler cache can be sized without a config file change. The value is resolved in `OptsHandler` alongside the other env-backed options, so the configuration file keeps taking precedence over the environment variable, and defaults to the pre-existing 250. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * test(backend-shared): align CUBEJS_COMPILER_CACHE_SIZE assertions with env-var `asIntPositive()` accepts 0 (it rejects negatives and non-integers), which matches the `min(0)` Joi rule the `compilerCacheSize` option already uses. Assert on the values it actually rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * fix(backend-shared): reject CUBEJS_COMPILER_CACHE_SIZE=0 instead of silently defaulting env-var's `asIntPositive()` lets 0 through, but every consumer of the option falls back to the default on a falsy value (`server.ts:205`, `server.ts:750`, `CompilerApi.ts:163/168/173`), so `CUBEJS_COMPILER_CACHE_SIZE=0` was accepted as valid and then quietly became 250. Fail loudly instead, and say so in the docs, so 0 can't be mistaken for a way to disable the compiler cache. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * test(backend-shared): isolate CUBEJS_COMPILER_CACHE_SIZE invalid-value cases Split the bundled invalid-value assertions into a test.each, matching timezone.test.ts, so a regression in one input doesn't mask the others. Hoist the env cleanup into beforeEach so the suite is hermetic without each test repeating the delete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * fix(server-core): reject compilerCacheSize of 0 in the config file too The env variable now rejects 0, but `optionsValidate.ts` still accepted it from the config file and let it be silently replaced by 250 through the `|| 250` guards — the same trap, on the other entry point. Tighten the Joi rule to `min(1)`, matching `scheduledRefreshConcurrency` and `scheduledRefreshBatchSize` directly above it, so both entry points agree with the documented range. 0 has never sized the cache to anything but the default, so nothing can depend on its behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * test(backend-shared): drop redundant beforeEach in compilerCacheSize suite The afterEach already leaves the variable unset for the next test, so the beforeEach only relocated the redundancy flagged earlier rather than removing it. The sibling describe blocks in this file use afterEach alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb * docs: document that CUBEJS_COMPILER_CACHE_SIZE also sizes the SQL API cache The name is not new: CubeSQL already reads CUBEJS_COMPILER_CACHE_SIZE (rust/cubesql/cubesql/src/config/mod.rs:192) to size its own compiler LRU, defaulting to 100 rather than 250. Document both consumers and both defaults, and note that the compilerCacheSize config option sizes only the data model compiler cache, since CubeSQL reads the environment variable directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsJnAHifeU7RMbdumCLSDb --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )