[pull] master from cube-js:master - #669
Merged
Merged
Conversation
…1569) * test(trino-driver): smoke-test LIKE filter escaping against real Trino Trino has no default escape character for LIKE, so a `contains` filter over a value like `%` only works if the schema compiler both escapes the value (`%` -> `\%`) and emits an explicit `ESCAPE '\'` clause. Nothing covered that end to end, so a regression in either half would have silently turned user-supplied `%`, `_` or `\` back into wildcards. Adds an integration test that boots a real Trino, builds the query with the real PrestodbQuery (the dialect TrinoDriver.dialectClass() resolves to) and runs it through the real driver, for contains/notContains/startsWith/ endsWith on both the legacy and the Tesseract planner. Verified the test fails as intended: dropping the `ESCAPE '\'` clause from PrestodbQuery turns 14 of the 16 cases red on both planners. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(testing): drive Trino LIKE escaping through the REST API Replaces the driver-level LIKE escaping test with one that goes through the REST API against a real Cube server, so the check covers query planning as a user actually hits it rather than the schema compiler in isolation. That change of altitude is what surfaced the actual defect. Filters are only escaped by the dialect that defines the `filters/like_escape_char` template, and PrestodbQuery is the only one that does. A query answered from a pre-aggregation is planned for Cube Store instead, the native planner skips escaping when the template is absent, and the user's `%` reaches the pattern as a wildcard: `contains '%'` matches every row and `notContains '%'` matches none. The fixture therefore carries two cubes over identical data - one queried straight from Trino, one behind a rollup - and a guard asserts they really are planned for different engines, so the Cube Store path cannot silently stop being exercised. The three filters the rollup path gets wrong are marked `it.failing`: they document the defect without reddening CI, and will demand attention by failing once the escaping gap is closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(schema-compiler): escape LIKE wildcards on the native planner A `contains` filter is supposed to match a literal `%`, but on the native planner it matched every row instead. Escaping there only happens when the dialect defines the `filters/like_escape_char` template - the planner skips it entirely when absent - and PrestodbQuery was the only dialect that defined it. Every other dialect passed the user's value through untouched, so `%` and `_` reached the pattern as wildcards. `contains '%'` matched everything and `notContains '%'` matched nothing. This bites hardest where it is least visible: a query answered from a pre-aggregation is planned for Cube Store, so filters that were correct against the source database silently changed meaning once a rollup started serving them. Declares the escape character on the base templates, so every dialect inherits the escaping the legacy planner has always applied via BaseFilter.escapeWildcardChars. It is a bare character with no ESCAPE clause because backslash is already the default in Postgres, MySQL, BigQuery, ClickHouse and Cube Store - and Cube Store's parser rejects an explicit clause outright, so a clause in the shared template would break every pre-aggregation query. Dialects with no default escape character need the clause to interpret that escaping, so MSSQL, Oracle and Snowflake now emit one on the native filter path, matching what each already does on the legacy path. Oracle's was present but gated on `default_escape`, which the filter path never sets. Verified end to end against real Trino and real Cube Store, and by SQL shape across fifteen dialects on both planners. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(schema-compiler): cover DuckDB and Pinot, and run the smoke test in CI Follow-up to review on #11569. DuckDB and Pinot were missed when auditing which dialects need an explicit ESCAPE clause, because both live in driver packages rather than in schema-compiler's adapter directory, and the sweep only covered the latter. DuckDB gates its `expressions.like` on `default_escape`, which is how this repo records that a dialect has no default LIKE escape character, and Pinot already emits the clause on its legacy filter path. Both therefore ended up escaping the value with nothing to interpret it, turning `contains '%'` from matching everything into matching nothing. The new smoke test was also unreachable from CI: `.github/actions/smoke.sh` invokes each suite by explicit script name, so a script that is not listed there only ever runs locally. A regression guard nothing runs guards nothing, so it now sits alongside `smoke:trino`. Also pins the MSSQL legacy case in the unit test instead of branching around it. It was asserting nothing while still counting as a passing test, which made the suite look like it covered a case it did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(duckdb-driver,pinot-driver): pin the LIKE escape clause Follow-up to review on #11569. The DuckDB and Pinot ESCAPE clauses were the only lines in the change with nothing asserting on them, so a later template edit could drop either and reintroduce exactly the bug the rest of the work exists to prevent. The dialect matrix in schema-compiler cannot reach them - it sits upstream of the driver packages - so each driver gets the assertion in its own package, covering both planners. Also names DuckDB and Pinot in BaseQuery's enumeration of dialects that carry an explicit clause. That comment is what a future sweep reads to find them, and it is precisely what failed here: both live outside src/adapter/, so enumerating that directory missed them. Keeps PrestodbQuery's now-duplicate like_escape_char rather than inheriting it, with a comment explaining why: the ESCAPE clause in its like_pattern hardcodes the same character, so the two have to change together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(duckdb-driver,pinot-driver): make the template tests reachable from CI Follow-up to review on #11569. Both new template tests were written where nothing would run them. Pinot's `unit` script matches `dist/test/unit`, so a file directly under `test/` is excluded, and nothing invokes `integration:pinot` — there is no Pinot group in smoke.sh. The DuckDB one was reachable, but only through `integration:duckdb` in the docker smoke job, which is a slow place to run a test that needs no database. Moving both under `test/unit/` puts them in the fast `lerna run unit` job. DuckDB needs a `unit` script for that, since it had none. Confirmed by running the CI entry points rather than the files directly: pinot yarn unit -> 2 suites, 11 tests duckdb yarn unit -> 1 suite, 2 tests This also rescues Pinot's pre-existing sql_table template test, which has never run in CI for the same reason. A guard nothing runs guards nothing — the same gap as the smoke script wiring, one level down, and worth checking by execution rather than by reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(testing-drivers): cover LIKE wildcard escaping across every driver The escaping fix changes behaviour for every dialect, but the only behavioural coverage was Trino. These run in the shared driver suite, so they execute against all thirteen engines the suite supports, and they exercise both escaping paths: once against a cube with no pre-aggregations, so the filter reaches the database, and once through a query the ECommerce `SA` rollup serves, where the filter SQL is generated for the rollup store instead. The rollup path is the one that broke in production. No fixture data was added. The existing dataset already discriminates: no product name contains a percent sign, so `contains '%'` must return nothing and would return everything if the wildcard leaked, and exactly one product has a literal underscore, so `contains '_'` must return that one rather than all twenty-eight. Asserts exact result sets rather than snapshots, deliberately. The wrong answer here is a superset of the right one, so a snapshot would have recorded the broken result as expected — which is how this class of bug survives. Note the pre-existing `contains with special chars` test does not discriminate: `%di_Novo%` matches the same single row whether or not the underscore is escaped, so it would have stayed green throughout. Verified against real Postgres on the native planner: 124 passed, including all five, with the rollup queries confirmed to route to the Cube Store pre-aggregation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(testing-drivers): pin which engine answers each LIKE escaping case Follow-up to review on #11569. The pre-aggregated pair named the rollup path but never checked it, and `contains '%'` cannot notice the difference on its own: it returns nothing whether the rollup or the source database answers. If `SA` ever stopped matching, both cases would keep passing as duplicates of the Products ones and the rollup-store escaping path would lose its only coverage here, silently - the same shape as the two CI-reachability gaps already fixed on this branch. Asserts `external` on all four cases, in both directions: false for the cubes with no pre-aggregations, true for the ones the rollup serves. Pinning both halves is what makes the pair a contrast rather than two similar queries. The review suggested `usedPreAggregations`, which is the more explicit field and is what the Trino smoke test would use. It is not available here: the gateway only emits it in dev mode, and this suite does not enable it, so that assertion would have failed on all thirteen engines. Confirmed by dumping the real `/load` response, which carries `external: true` and `extDbType: cubestore` instead. Verified against real Postgres: 124 passed, with the guards active. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(testing-drivers): assert the engine on the notContains case too Follow-up to review on #11569. It was the only one of the four Products cases not pinning which engine answered, and the only one issuing two concurrent loads - so the one most likely to be edited later without noticing the omission. Harmless today, since Products has no pre-aggregations in any fixture, but the point of these guards is that the pairing cannot quietly stop being a pairing. Verified against real Postgres: 124 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(pinot-driver): drop the ESCAPE clause, which Pinot rejects The new driver test caught this: `contains '_'` returned nothing on Pinot where it should have returned the one product with a literal underscore. Verified against Pinot 1.4.0 rather than inferred. An explicit clause fails at execution: 'a_b' LIKE '%\_%' ESCAPE '\' -> Query execution error 'a_b' LIKE '%\_%' -> matches 'axb' LIKE '%\_%' -> no match So Pinot already reads backslash as the LIKE escape character and rejects being told so. The escaping the planner applies to the value takes effect precisely by emitting no clause. Removes it from both paths. The clause on the legacy filter predates this branch and has never worked - nothing asserted that a literal `_` matches, so it went unnoticed; the earlier commit here propagated the same mistake to the native path. The unit test now pins the absence of the clause as well as the escaping of the value, since restoring either half alone breaks it. One Pinot limitation remains and is not fixable from this side: an escaped `%` immediately followed by a trailing wildcard is mis-parsed, so `'50% off' LIKE '%\% off'` matches but `'%\%%'` does not. `contains '%'` therefore cannot match a literal percent sign on Pinot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * revert(pinot-driver): split Pinot out of the escaping fix The Pinot job on b6a6465 disproved the theory that commit was built on. Removing the ESCAPE clause did not restore Pinot's LIKE matching: Snapshots: 102 passed, 102 total <- the 21 `Array []` did not change Tests: 1 failed, 110 passed filtering Products: contains a literal underscore -> Received [] So the empty snapshots are accurate rather than stale. Pinot's LIKE does not match a non-constant CONCAT(...) pattern, which is why contains, startsWith and notContains all return nothing there - independently of escaping, and since well before this branch. Dropping the ESCAPE clause was necessary but nowhere near sufficient, and fixing the rest means changing how the pattern is built for that dialect, which is its own piece of work and needs a Pinot to test against. Reverts PinotQuery.ts to master so this PR carries only the escaping change, and skips the one new case Pinot cannot pass, with the reason recorded in the fixture's own banner style. The other four escaping cases still run there - they pass because they expect an empty result, which is noted so nobody reads those ticks as Pinot being correct. The test-file move to test/unit/ stays: it is a CI-reachability fix, not a Pinot behaviour change, and it rescues the pre-existing sql_table test that had never run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * docs(testing-drivers): correct what the Pinot banner claims about notContains The banner said the other escaping cases pass on Pinot "only because they expect an empty result". That holds for the two contains-a-literal-percent cases and not for notContains, which asserts a non-empty result equal to the unfiltered count and passes because NOT LIKE over a never-matching pattern returns every row. It is the one Pinot case here whose assertion still means something, and describing it as vacuous invites the next reader to skip it. The banner is what someone reads before adding a name to this list, so it is worth being exact about which cases are load-bearing. Also points at #11570 rather than saying "tracked separately". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * docs(testing-drivers): split the Pinot banner by which engine answers The previous pass fixed one mis-classification and introduced its mirror. Calling both contains-a-literal-percent cases vacuous is wrong: the ECommerce (pre-aggregated) one asserts servedByRollupStore true, so Cube Store generates and evaluates that pattern and Pinot's LIKE never runs. It is as protective here as on any other engine. And notContains is not the strongest surviving case - the pre-aggregated underscore case asserts a specific non-empty row from the rollup store. What decides whether a case means anything on Pinot is which engine answers it, not the shape of its assertion, so the banner now splits on that. It sits directly above a skip list, and labelling the two genuinely protective cases as vacuous is the one error most likely to cause real damage there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * test(testing-drivers): move Trino LIKE escaping coverage into the shared suite The Trino LIKE-escaping regression was covered by a standalone birdbox smoke test in cubejs-testing. That put the only end-to-end proof of the fix in a Trino-specific file, so no other engine exercised it and the driver suite - where every backend runs the same cases - stayed blind to the bug. Onboard Trino as a driver in cubejs-testing-drivers and delete the smoke test: - fixtures/trino.json: trinodb/trino:476 against the in-memory catalog, the Presto-family cast rules, and the same pre-aggregation set the other fixtures declare. The container raises query.max-stage-count because the fixture tables are built from ~1000-branch UNION ALL chains, which blow past Trino's 150-stage default. - runEnvironment: wait on the image's HEALTHCHECK for Trino. The compose environment overwrites each strategy's own timeout with the global one, so the global value is what has to be raised. - Both planners are wired into drivers-tests.yml. Skip lists are split by planner and annotated with the Trino limitation behind each entry: no WEEK interval unit, PrestodbQuery rejecting multi-part intervals, ordinal GROUP BY in SQL API rollup push-down, and unordered results that cannot be snapshotted. Locally: 128 passed / 11 skipped with Tesseract, 109 passed / 30 skipped on the legacy planner, no failures on either. The shared suite also picks up startsWith and endsWith percent-sign cases plus a pre-aggregated notContains, so the operator coverage the smoke test had is kept, now running for every driver rather than Trino alone. Verified green on Postgres under both planners. buildPreaggs now rejects with the response body when the jobs endpoint answers with an error instead of a token array. That path previously died on an opaque TypeError inside a polling interval and failed 120s later complaining about `tokens`, which hid the pre-aggregation the fixture was actually missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(testing-drivers): reject with the response body when the jobs endpoint is not JSON Two follow-ups from review on the pre-aggregation build helper. A response that is not JSON at all - a proxy error page, or an empty body from a cube that died during startup - threw inside a `.then` with no rejection handler. The outer promise never settled, and the 120s backstop is only armed once there are tokens to poll, so the build hung until jest's own timeout with the response never printed. The parse is now guarded and rejects with the body, and the chain carries a `.catch` so a failed request settles too. The polling round had the same hole one step later, where the backstop reduced it to `timeout.` with no cause. Guarded the same way. The backstop is also cleared on the success path now, instead of being left pending. Also correct the Pinot banner, which the previous commit invalidated: the escaping group grew to three Cube Store-answered ECommerce cases, and startsWith and endsWith join contains as vacuous on Pinot, so naming contains "the only vacuous one" no longer held. Postgres driver suite re-run green after the rewrite: 127 passed, 20 skipped, with `must built pre-aggregations` still building all eight rollups. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(testing-drivers): stop polling once the pre-aggregation build has settled The `failure` branch rejected from inside the status tally, where a `failure` status is neither `done` nor `missing_partition`, so `inProcess` stayed non-empty and neither timer was cleared. The helper went on POSTing to the jobs endpoint once a second for the remaining ~120s after the promise had settled, then the backstop called reject on an already-settled promise. `--forceExit` kept that from stalling the run, but it interleaved the polling output with every following test at exactly the moment someone is reading it to find out why the build failed. Hoist the check out of the loop so it settles and stops, and route every settle path through one `stop()` that clears both timers - the two guarded rejects added in the previous commit cleared the interval but left the 120s backstop pending. Postgres driver suite re-run green: 127 passed, 20 skipped, pre-aggregations still building. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(testing-drivers): count outstanding partitions across the whole job list `hookPreaggs` is the twin of the loop the previous commit fixed, and had the same defect plus a worse one. `postBuildJobs` returns one token per partition and the outstanding tally was tested inside the loop over them, so a first token reporting `done` resolved the build while later partitions were still scheduled - the suite then queried a rollup table that did not exist yet. Count over the whole array instead. The `failure` branch also rejected without stopping the poll, and an empty job list never settled at all because the check lived in a loop body that never ran; both follow from the same restructuring. Verified with the postgres-core suite, which is what exercises this function: 6 passed, snapshots unchanged, so the recorded driver calls are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF * fix(testing-drivers): guard the jobs poll in hookPreaggs like its twin The `await preAggregationsJobsGET` was the last unguarded one in this file. The orchestrator and compiler errors it can raise became unhandled rejections thrown out of an async interval callback, so `stop()` never ran, the poll kept going, and the build died 60s later on `timeout.` with the real error never printed - the failure mode already removed from `buildPreaggs`. The two poll loops now have the same shape. postgres-core green again: 6 passed, snapshots unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDVoT5YNCrewPZ87jyzmkF --------- Co-authored-by: Claude <noreply@anthropic.com>
github-actions
Bot
added
data source driver
driver:duckdb
driver:pinot
javascript
pr:community
labels
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 : )