Skip to content

fix(pinot-driver,dremio-driver,druid-driver): interpret LIKE wildcard escaping - #11811

Open
waralexrom wants to merge 10 commits into
masterfrom
tesseract-like-pattern-escaping
Open

waralexrom wants to merge 10 commits into
masterfrom
tesseract-like-pattern-escaping

Conversation

@waralexrom

@waralexrom waralexrom commented Sep 9, 2026

Copy link
Copy Markdown
Member

Problem

A contains / notContains / startsWith / endsWith filter is supposed to match the
user's value literally: searching for 50%_off must return the rows containing that
string, not every row. That takes two cooperating pieces, and both have to come from the
dialect:

  1. the %, _ and the escape character itself are escaped inside the value;
  2. the emitted statement interprets that escaping — either because the engine reads
    backslash as its default LIKE escape character, or because the statement carries an
    explicit ESCAPE clause.

Having (1) without (2) is the dangerous half: the backslash stops being an escape and
becomes plain data, so the pattern searches for a string nobody has and the filter
silently returns nothing.

The escape character is declared once on the base templates, so escaping now happens for
every dialect. A dialect whose LIKE has no default escape character therefore has to say
so. Three did not, in four places:

dialect planner before
Pinot native LOWER(x) LIKE CONCAT('%', LOWER(?), '%') — legacy already had the clause
Dremio native x ILIKE '%' || ? || '%' — an operator Dremio does not have
Dremio legacy ILIKE(x, CONCAT('%', ?, '%')) — the function takes no escape argument
Druid both LOWER(x) LIKE CONCAT('%', LOWER(?), '%')

Dremio's legacy path carried a second defect in the same function: the negation was
spliced into the first argument of the call — ILIKE(x NOT, CONCAT(...)) — a parse error
rather than a filter, for every notContains, notStartsWith and notEndsWith.

Cause

The native filter path renders tesseract.ilike, which is a separate template from
the expressions.like / expressions.ilike pair the SQL API push down uses. A dialect
that carries the clause on the push-down path — Dremio and DuckDB both do, gated on
default_escape — still has nothing on the filter path.

Pinot was missed even though BaseQuery's enumeration of the dialects that carry a clause
already named it; Dremio and Druid were never in that enumeration at all.

What changed

  • Pinot (tesseract.ilike): carries ESCAPE '\', matching what
    PinotFilter.likeIgnoreCase already emits. It cannot go inside like_pattern because
    Pinot's pattern is wrapped in CONCAT(...).

  • Dremio (tesseract.ilike, new; and DremioFilter.likeIgnoreCase): the case folding
    moves from the ILIKE(expr, pattern) function onto LOWER(...) LIKE LOWER(...), since
    the function takes no escape argument and LIKE does. That also puts the negation beside
    the operator, fixing the parse error.

  • Druid (tesseract.ilike and DruidFilter.likeIgnoreCase): both carry the clause.
    The changed SQL is what its dialect test pins, so that assertion moves with it. The file
    also deleted expressions.like_escape — how a dialect declares that its LIKE takes no
    ESCAPE clause, and what the SQL API push-down rewrite is gated on — five lines above
    emitting that clause. Asked directly, Druid takes it, so the deletion was the stale half
    and goes; LIKE with an escape sequence can now push down instead of falling back.

    name LIKE '%a\b%' ESCAPE '\'   ->  a\b
    name LIKE '%a\b%'              ->  (nothing)
    name LIKE 'a%b'  ESCAPE '\'    ->  aXb, a\b
    
  • BaseQuery: the comment enumerating which dialects carry an explicit clause — the
    thing a future sweep of this area reads — names Dremio and Druid, and names ksqlDB as
    the one dialect the shared escape character is wrong for.

No Rust change. The native planner's own escaping is correct and already covered by unit
tests in cubesqlplanner; this is entirely about the dialect templates it renders through.

How it was verified

By values, against real Postgres. Both planners were run over a table holding
50%_off, 50Xyoff, a\b, aXb, plain, 50%_offer for seven filter shapes. Every
case matches literally and the two planners agree:

contains '%'           MATCH  legacy=["50%_off","50%_offer"]  tess=["50%_off","50%_offer"]
contains '_'           MATCH  legacy=["50%_off","50%_offer"]  tess=["50%_off","50%_offer"]
contains '50%_off'     MATCH  legacy=["50%_off","50%_offer"]  tess=["50%_off","50%_offer"]
contains 'a\b'         MATCH  legacy=["a\\b"]                 tess=["a\\b"]
startsWith '50%'       MATCH  legacy=["50%_off","50%_offer"]  tess=["50%_off","50%_offer"]
endsWith '_off'        MATCH  legacy=["50%_off"]              tess=["50%_off"]
notContains '%'        MATCH  legacy=["50Xyoff","aXb","a\\b","plain"]  tess=[same]

By values, against real Druid. Druid already has a cluster in CI - its own
docker-compose, reached from the integration matrix - but nothing there built a query
through DruidQuery, so the filter family was covered by SQL shape only. It now ingests
rows and asserts exact result sets on both planners. That also settles the one question
the shape tests cannot answer: Druid does honour ESCAPE on a non-literal pattern, but
only over a datasource - over an inline SELECT 'x' AS name it refuses the query with
Function[like] pattern argument must be a literal, which is why the case ingests instead
of selecting constants.

Undoing the clause turns 10 of those 12 cases red against the live cluster: rows expected,
Array [] returned, and notContains '%' returning every row instead of three. The
ordinary-value case stays green, so the escaping cannot be "fixed" by breaking plain
search.

Value-level coverage across every engine the shared driver suite supports, on both the
source-database and the rollup-store escaping paths, already exists in
cubejs-testing-drivers. Pinot is in that suite, but its LIKE cases cannot discriminate:
it does not match a non-constant CONCAT(...) pattern at all (tracked separately). Dremio
is the only one of the three with no engine to test against - it has no compose file and
is commented out of the integration matrix as flaky.

By SQL shape, across every dialect in the repo, for all six LIKE operators on both
planners. Pinot, Dremio and Druid now emit the same predicate on each planner. Those checks
were run while developing the fix but are not committed: coverage here rests on behaviour,
not on the SQL text, and shape assertions largely restate the templates they read.

The value cases fail without the fix. Undoing the clause turns 10 of the 12 Druid cases
red, as above.

Note the coverage this leaves per dialect, which is uneven and worth knowing at merge time:

dialect engine available what covers the clause
Druid own docker-compose cluster, in the integration matrix the value cases added here, both planners
Pinot in cubejs-testing-drivers nothing effective - its LIKE cases cannot discriminate while it refuses non-constant patterns
Dremio none - no compose file, commented out of the matrix as flaky nothing

Risks

  • Druid is now verified by values against a real engine. Pinot and Dremio still rest on
    the repo's own record. For Pinot that record is strong: the exact SQL form is what its
    legacy path already sends to real Pinot, pinned by an existing driver test. For Dremio it
    is sqlTemplates deleting expressions.ilike (no infix operator) plus the
    default_escape gate on expressions.like (no default escape character), and an
    existing driver test that already assumes the LOWER(...) LIKE ... ESCAPE '\' shape.
  • Dremio's native path could not parse before this change and its legacy path could not
    parse for any negated operator, so neither can regress into a worse state.
  • Druid's legacy SQL changes shape for every LIKE filter. LOWER(x) LIKE p ESCAPE '\' is
    the same predicate it emitted before plus the clause, so the only behaviour that changes
    is the one that was wrong.

Left open, deliberately

  • ksqlDB overrides filters.like_pattern but inherits like_escape_char, and its
    LIKE/ILIKE accept no ESCAPE clause at all — so the injected backslashes are matched
    as data and no clause can rescue them. The lever is to stop escaping there rather than to
    add a clause, which is a behaviour change in the opposite direction; with no ksqlDB
    available to confirm its LIKE semantics, this is recorded in the BaseQuery comment instead
    of changed blind.
  • DuckDB's legacy path has the mirror-image gap: it escapes the value and emits no
    clause, while its native path carries one.
  • Hive and SQLite render the base infix ILIKE on the native planner, which neither
    engine has. Different defect class — the statement does not parse rather than quietly
    returning the wrong rows — and not about escaping.

🤖 Generated with Claude Code

@waralexrom
waralexrom requested review from a team as code owners September 9, 2026 10:18
@github-actions github-actions Bot added driver:druid Issues relating to the Druid driver driver:dremio javascript Pull requests that update Javascript code data source driver driver:pinot labels Sep 9, 2026
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 5m 23s —— View job


Review: sound fix, well-targeted tests — 1 medium (Druid may not accept a non-literal LIKE pattern), 4 low (test strictness, comment volume ×3). No blocking correctness bug found.

Full review — click to expand
  • Read PR diff (git diff origin/master...HEAD)
  • Verify test wiring (module paths, jest config, unit scripts, CI reachability)
  • Verify SQL template correctness per dialect
  • Check existing review threads / dedupe (none existed)
  • Post review

What I verified independently

  • The escaping/interpretation pairing is real. BaseFilter.escapeWildcardChars (BaseFilter.ts:112) escapes \, _, % for every LIKE operator on the legacy path, and the native path does the same in like.rs::escape_like_pattern gated on like_escape_char. Since all three dialects inherit like_escape_char: '\\' and none deletes it, the unconditional ESCAPE in tesseract.ilike is consistent with the value always being escaped — no risk of emitting a clause the value doesn't need.
  • Test wiring resolves. Dremio's require('../../../driver/DremioQuery') is correct from dist/test/unit (rootDir: ".", driver/ is uncompiled JS) and matches the sibling params-escaping.test.ts. Druid's ../../src/DruidQuery resolves to dist/src/DruidQuery.js. Both new files are reached by jest dist/test/unit.
  • CI really runs both planners. .github/workflows/push.yml:137 runs yarn lerna run unit after yarn tsc, with the native addon downloaded as an artifact — so useNativeSqlPlanner: true exercises Tesseract rather than silently falling back. The new Druid unit script is needed exactly as described (integration is jest dist/test/*.js, top-level only).
  • No collateral snapshots. Nothing else pins Dremio's old ILIKE (col NOT, ...) shape, and Pinot's LIKE-family cases are already skipped in fixtures/pinot.json for the non-constant-pattern reason, so the changed Pinot template causes no snapshot churn.
  • Dremio's legacy negation fix is real. ILIKE(col NOT, ...) was a parse error for every notContains/notStartsWith/notEndsWith; moving to LOWER(col) NOT LIKE ... puts the negation beside the operator. Good catch, and the test pins it.

Findings

# Severity Where Issue
1 Medium DruidQuery.ts:22 Druid may not accept a non-literal LIKE pattern — same limitation the PR cites for Pinot — in which case the clause makes the SQL look right without fixing the rows returned
2 Low new unit tests (×3) toContain("ESCAPE '\\'") doesn't tie the clause to the LIKE predicate; only contains is covered, and the negated shape is unpinned for Pinot and Druid
3 Low DremioQuery.js:175-183 8-line comment restates the 5-line one at lines 16-20 sentence for sentence
4 Low PinotQuery.ts:256 Stray space renders LOWER(x) LIKE / NOT LIKE; comment duplicates the BaseQuery block
5 Low BaseQuery.js:4782-4787 Comment now ~24 lines and doubles as the tracker for a known-broken dialect (ksqlDB) — that belongs in an issue

Details are in the five inline comments.

Comment volume, generally

Beyond the three flagged inline, each new test file opens with a 5-6 line prose preamble arguing the same point (DruidQueryTemplates.test.ts:33-38, PinotQueryTemplates.test.ts:53-57, DremioQueryTemplates.test.ts:52-57, plus two more in the Dremio file). Five copies of one rationale spread across three packages. The test names already say what's asserted; the one thing worth keeping near an assertion is the non-obvious bit — this dialect has no default escape character — which fits on one line.

Not run

I couldn't execute the new tests: this checkout has no node_modules and a full monorepo install plus yarn tsc across schema-compiler and the three drivers wasn't available here. The review is static, plus the PR's own recorded failure output. If you want the suites run, allowlisting yarn install/yarn tsc would let me do that.

· branch [`tesseract-like-pattern-escaping`](https://github.com/cube-js/cube/tree/tesseract-like-pattern-escaping)

Comment thread packages/cubejs-druid-driver/src/DruidQuery.ts
Comment thread packages/cubejs-dremio-driver/driver/DremioQuery.js Outdated
Comment thread packages/cubejs-druid-driver/test/unit/DruidQueryTemplates.test.ts Outdated
Comment thread packages/cubejs-pinot-driver/src/PinotQuery.ts Outdated
Comment thread packages/cubejs-schema-compiler/src/adapter/BaseQuery.js Outdated
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.26%. Comparing base (1699910) to head (0a67f6e).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11811      +/-   ##
==========================================
+ Coverage   60.16%   60.26%   +0.09%     
==========================================
  Files         239      239              
  Lines       19206    19178      -28     
  Branches     3886     3881       -5     
==========================================
+ Hits        11556    11558       +2     
+ Misses       7100     7070      -30     
  Partials      550      550              
Flag Coverage Δ
cube-backend 60.26% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 2m 37s —— View job


Re-review after 3 new commits: all five prior findings addressed. 1 new medium — DruidQuery.ts deletes expressions.like_escape while adding an ESCAPE clause four lines below.

Full review — click to expand
  • Read the diff since my last review (commits 2aecc55, c60a1ac, 8b58d04, 1738851)
  • Re-check the five prior findings against the current code
  • Resolve my stale review threads (5 resolved)
  • Post updated review

Prior findings — status

# Finding Status
1 Druid non-literal LIKE pattern unverified RecordedDruidQuery.ts:14-15 now says so explicitly ("Unverified against a real broker"), which was the fallback I asked for
2 Tests only pinned ESCAPE '\' as a floating substring, contains only Fixed — all three files now pin the whole predicate and parameterise over contains/notContains/startsWith/endsWith on both planners (8 cases each)
3 Duplicate 8-line Dremio comment Fixed — cut to the 2-line pointer, verbatim from the suggestion
4 Pinot stray space + duplicated comment Fixed{% endif %}LIKE now, comment down to 2 lines
5 BaseQuery comment ~24 lines Improved — 14 → 10 lines. ksqlDB is still tracked in the comment rather than an issue; low, and your call

The tightened tests are a genuine improvement over what I asked for: pinning LOWER("orders".status) NOT LIKE LOWER(CONCAT('%', ?, '%')) ESCAPE '\' catches both the clause drifting and the negation landing back inside the call, and the does not render ILIKE case guards the Dremio operator staying gone on both planners.

Newly verified this round

  • Dremio's new legacy shape agrees with what the package already expects. test/unit/params-escaping.test.ts:9 independently asserts LOWER(name) LIKE '%' || LOWER(?) || '%' ESCAPE '\' — same construct the rewritten DremioFilter.likeIgnoreCase now produces. Nothing anywhere pins the old ILIKE (col NOT, …).
  • Druid's new unit script is wired. tsconfig.json has include: ["src", "test"], rootDir: ".", so test/unit/*.ts compiles to dist/test/unit; jest and @types/jest are already devDependencies and schema-compiler a dependency. integration's dist/test/*.js glob really would have missed the directory.
  • The escape/clause pairing still holds on both paths. BaseFilter.escapeWildcardChars (BaseFilter.ts:112) escapes unconditionally for every operator isWildcardOperator covers, and the native side keys off like_escape_char, which none of the three deletes — so the unconditional clause never over-escapes.

New finding

Medium — packages/cubejs-druid-driver/src/DruidQuery.ts:68. delete templates.expressions.like_escape is how a dialect declares its LIKE takes no ESCAPE clause (CubeSQL gates the push-down rewrite on the template's presence, like_expr.rs:108), and line 72 now emits precisely that clause. If Druid does accept ESCAPE the deletion is stale and needlessly blocking push-down; if it doesn't, the new clause turns every LIKE filter into a parse error. Details inline.

This is distinct from the non-literal-pattern caveat already recorded in the DruidFilter comment: that one is about whether the clause is honoured, this about whether it parses.

Not run

Same limitation as last round — no node_modules in this checkout, so the new suites were not executed. The sql.replace(/\s+/g, ' ') normalisation plus exact-predicate toContain makes the expectations whitespace-fragile against template edits by design, so I'd want a real run of yarn lerna run unit --scope '@cubejs-backend/{druid,dremio,pinot}-driver' before merge to confirm the eight Dremio strings (particularly the ?|| '%' spacing quirks) match what the base like_pattern actually renders.

· branch [`tesseract-like-pattern-escaping`](https://github.com/cube-js/cube/tree/tesseract-like-pattern-escaping)

Comment thread packages/cubejs-druid-driver/src/DruidQuery.ts Outdated
waralexrom and others added 8 commits September 10, 2026 18:38
…ative planner

Both planners escape `%`, `_` and the escape character itself inside a
user-supplied LIKE value, but that escaping only means anything if the emitted
statement carries the clause that interprets it. Pinot and Dremio both have no
default LIKE escape character, and nothing asserted on either dialect's native
filter path, so a value like `50%_off` could silently stop matching literally
without a single test noticing.

Pinot's case covers both planners, since its legacy path already carries the
clause. Dremio's covers the native planner, where the statement additionally
has to avoid the infix `ILIKE` operator the dialect does not have.

Both tests fail as written:

    pinot   ✕ escapes LIKE wildcards ... on the tesseract planner
            Expected substring: "ESCAPE '\'"
            Received: WHERE ((LOWER("orders".status)  LIKE CONCAT('%', LOWER(?), '%')))

    dremio  ✕ interprets that escaping with an explicit ESCAPE clause
            ✕ does not render ILIKE as an infix operator
            Received: WHERE (("orders".status ILIKE '%' || ?|| '%'))

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e planner

A `contains`, `startsWith` or `endsWith` filter is supposed to match a literal
`%` or `_`, and the value is escaped for exactly that. Neither Pinot nor Dremio
has a default LIKE escape character, so on those two dialects the escaping the
native planner applies reached the engine with nothing to interpret it: the
backslash stayed a plain character and a search for `50%_off` matched nothing
instead of the rows containing it.

Pinot's legacy filter path already emits the clause, so the two planners
disagreed on the same query. Dremio's native path was worse off: it also
inherited the base infix `ILIKE` operator, which the dialect does not have -
that is what deleting `expressions.ilike` records - so it could not carry the
clause and would not parse. Its case folding therefore moves onto `LOWER(...)
LIKE LOWER(...)`, which takes an ESCAPE clause where the `ILIKE(expr, pattern)`
function does not.

The escape character is declared once on the base templates, so a dialect only
has to say that its LIKE needs the explicit clause. BaseQuery's enumeration of
the dialects that carry one is what a future sweep reads, so it names Dremio
too.

Verified by values against real Postgres, where both planners return the rows
containing a literal `%`, `_` and `\` rather than every row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…egacy planner too

Escaping a filter value only means anything if the emitted statement carries the
clause that interprets it, and that is true whichever planner emitted the
statement. Pinning only the native planner locks the divergence in rather than
catching it, and Druid was not covered at all.

Dremio's case now covers both planners, and adds one for the negation: a `NOT`
belongs beside the operator, not spliced into the first argument of a function
call. Druid's covers both planners; its test needs a `unit` script to be
reachable from the fast CI job, since `integration`'s top-level glob leaves the
directory out.

The added cases fail as written:

    dremio  ✕ escapes ... on the legacy planner
            ✕ does not render ILIKE on the legacy planner
            ✕ negates beside the operator on the legacy planner
            Received: WHERE ( ILIKE ("orders".status NOT, CONCAT('%', ?, '%'))
                             OR "orders".status IS NULL)

    druid   ✕ escapes ... on the legacy planner
            ✕ escapes ... on the tesseract planner
            Expected substring: "ESCAPE '\'"

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y planner too

Escaping a filter value with a backslash is only half of matching a literal `%`
or `_`; the statement still has to say that the backslash is the escape
character. Neither Dremio nor Druid has a default one, and both left that unsaid
on the legacy filter path, so `contains: ['%']` searched for a literal `\%` and
returned nothing.

Dremio also spliced the negation into the first argument of a function call -
`ILIKE(x NOT, CONCAT(...))` - which is a parse error rather than a filter, for
every notContains, notStartsWith and notEndsWith. Moving the case folding onto
`LOWER(...) LIKE ...` fixes both at once: unlike the ILIKE function, LIKE takes
an escape clause, and the negation lands beside the operator where it belongs.

Druid needed the clause on both planners, and its inherited native template had
none either. The `ESCAPE` clause changes the SQL its dialect test pins, so that
assertion moves with it.

BaseQuery's enumeration of the dialects that carry an explicit clause is what a
future sweep of this area reads, so it names Druid. It also names ksqlDB as the
one dialect the shared escape character is wrong for: its LIKE accepts no
ESCAPE clause at all, so no clause can rescue the backslashes and they are
matched as data. Left as is rather than fixed blind - suppressing the escaping
is a behaviour change in the opposite direction, and no ksqlDB is available here
to confirm it.

Verified by values against real Postgres (unchanged, both planners literal), and
by SQL shape for all six LIKE operators on both planners: Pinot, Dremio and
Druid now emit the same predicate on each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing comments back

The rationale for the escape clause ended up written out three or four times -
twice inside DremioQuery alone, sentence for sentence, and again in each dialect
on top of the BaseQuery block that already records the mechanism. One statement
of a reason protects a future edit as well as four do; past that it is prose
around a template value, and the BaseQuery block had grown to two dozen lines of
it. Each note is now the load-bearing sentence, and the enumeration of which
dialects carry an explicit clause - the part a sweep of this area actually
reads - survives intact.

Also drops the stray space in Pinot's template, which rendered `LOWER(x)  LIKE`
and `NOT  LIKE`. It made the native predicate differ from the legacy one by
whitespace alone, which any assertion on the whole predicate has to encode.

Records on DruidFilter that whether Druid honours ESCAPE on a non-literal
pattern is unverified here, so the next reader does not take this path for
confirmed. The clause is still strictly better than none: without it the
backslashes are matched as data on any code path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dicate, not just the clause

`toContain("ESCAPE '\'")` proves the substring exists somewhere in the
statement. It does not tie the clause to the LIKE predicate it has to attach
to, so it would stay green with the clause drifting outside the CONCAT(...) or
onto a different filter entirely. Each case now pins the whole predicate.

Coverage also stopped at `contains`, which left the wildcard placement of
`startsWith`/`endsWith` and every negated operator unasserted - and the negated
shape is exactly where Dremio's legacy parse error lived. All four operators now
run on both planners.

Confirmed the stronger form still catches the defect, by undoing the fix: 22 of
the 43 cases go red, against 4 under the previous assertions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…al Druid

Druid is the one of the three dialects with a real engine already in CI - its
own docker-compose cluster, reached from the integration matrix - but nothing
there built a query through DruidQuery, so the whole filter family was covered
only by SQL shape.

This settles a question the shape tests cannot answer: whether Druid honours
ESCAPE on a non-literal pattern. It does, but only over a datasource. Over an
inline `SELECT 'x' AS name` Druid refuses the query outright with `Function[like]
pattern argument must be a literal`, so the case ingests rows rather than
selecting constants - and waits for the rows to answer, since ingestion
finishing and the segment becoming queryable are separate events.

Asserts exact result sets against what a literal match means, in both
directions: `contains '%'` must return only the rows with a percent sign, and
`notContains '%'` only the rows without one. `contains 'off'` pins that an
ordinary value keeps working, so the escaping cannot be "fixed" by breaking
plain search.

Verified the cases catch the defect, by undoing the clause: 10 of the 12 go red
against the live cluster, with rows expected and `Array []` returned - and
notContains returning every row instead of three. `contains 'off'` stays green,
as it should.

    Tests: 16 passed (29s, ingestion included)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sts, keep the value ones

Coverage for the escape clause now rests on behaviour rather than on the SQL
text. Druid checks it by values against a real cluster; the shape assertions
restated the templates they were reading, so they said little the value cases do
not say better.

Pinot's file and Dremio's params-escaping cases predate this and stay as they
were. Druid's `unit` script existed only for the deleted file, and an empty
directory fails jest, so it goes with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@waralexrom
waralexrom force-pushed the tesseract-like-pattern-escaping branch from 036fb5d to e285dad Compare September 10, 2026 16:41
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 3m 18s —— View job


Re-review after 3 new commits: 1 medium — the last commit deleted the Pinot and Dremio tests, leaving both dialects' changes with no coverage. The new Druid value test is good and settles the open ESCAPE question. 1 low, 1 prior thread re-affirmed.

Full review — click to expand
  • Read the diff since the last round (99d56f5, 17444b5, 7dae88d, 3fe0ddd, e285dad)
  • Re-check prior findings against current code
  • Verify the new Druid value-test wiring (CI reachability, native addon, ingestion)
  • Resolve stale threads / dedupe (nothing stale — 5 already resolved, 1 still applies)
  • Post review

Prior findings — status

# Finding Status
1 Druid non-literal LIKE pattern unverified Fixed properly3fe0ddd ingests rows and asserts result sets against the real cluster on both planners, which answers it by behaviour rather than by comment
3 Duplicate 8-line Dremio comment Fixed — 2-line pointer
4 Pinot stray space + duplicated comment Fixed{% endif %}LIKE, 2-line comment
5 BaseQuery comment ~24 lines Fixed — 9 lines, enumeration kept, prose gone
2 Tests pinned only a floating ESCAPE '\', contains only Fixed in 7dae88d, then the tests were deleted in e285dad — see below
(last round) DruidQuery.ts:68 deletes expressions.like_escape while line 72 emits the clause Still open — code unchanged; not re-posting, the thread stands. The new value test now settles it: Druid parses and honours ESCAPE on a CONCAT(...) pattern, so the delete on line 68 is stale and is needlessly blocking SQL-API push-down of LIKE … ESCAPE. This PR is well placed to drop it

Verified this round

  • The Druid value test is reachable in CI and really runs Tesseract. druid is in the integration matrix (push.yml:424) with use_tesseract_sql_planner: true, and the job downloads the backend-native artifact before install — so useNativeSqlPlanner: true reaches buildSqlAndParamsRust (BaseQuery.js:918) rather than falling back. The file is picked up by the existing integration script (jest dist/test/*.js), which is why the unit script added last round could be dropped again.
  • The case set discriminates. contains '_' expecting one row is the sharp one: unescaped, %_% matches every non-empty row, so a missing clause turns 1 into 5. notContains '%' catches the inverse. And the contains 'off' case keeps the escaping from being "fixed" by breaking ordinary search.
  • Waiting on rows rather than on the task is the right call — segment handoff is a separate event from task success, and the try/catch/finally around a fresh driver per poll is correct.
  • prepareCompiler(..., { adapter: 'druid' }) is harmless — adapter is only declared on the options type in PrepareCompiler.ts:39 and not read.
  • Comment volume is now within the repo's 3-line guidance everywhere it was flagged.

Findings

Medium — packages/cubejs-dremio-driver/driver/DremioQuery.js:19 (inline). e285dad deleted DremioQueryTemplates.test.ts and DruidQueryTemplates.test.ts and reverted the additions to PinotQueryTemplates.test.ts. Dropping Druid's is right — the value test supersedes it. Dropping the other two is not: Druid is the only one of the three with an engine in CI, so for Dremio and Pinot the shape test was the only guard, and shape is what the fix consists of.

  • Dremio's likeIgnoreCase rewrite changes the SQL for every LIKE filter and fixes a parse error on every negated one. Dremio has no compose file and is commented out of the matrix (push.yml:426); test/unit/params-escaping.test.ts asserts on a hand-written string passed to applyParams and never calls DremioQuery. Nothing executes or asserts this method.
  • Pinot's tesseract.ilike clause is likewise unpinned — the surviving PinotQueryTemplates.test.ts covers only sql_table FROM rendering and LIMIT/OFFSET ordering, and the driver-suite LIKE cases are skipped in fixtures/pinot.json.

The deleted versions had already been tightened to pin the whole predicate across four operators on both planners, which is the form that catches both the clause drifting and the negation landing back inside the call. Worth restoring those two.

Low — packages/cubejs-druid-driver/test/druid-driver.test.ts:148 (inline). Credentials re-hardcoded next to config.url; and LIKE_ROWS lost the backslash value, so the escape character itself — the third thing escapeWildcardChars escapes — is uncovered.

Housekeeping. The PR description's "How it was verified" and "The tests fail without the fix — they are the first and third commits" now describe tests that are no longer in the branch.

Not run

No node_modules in this checkout, so neither the Druid integration suite (needs Docker + a ~4-minute ingestion) nor the driver unit suites were executed. Review is static plus CI-wiring inspection.

· branch [`tesseract-like-pattern-escaping`](https://github.com/cube-js/cube/tree/tesseract-like-pattern-escaping)

// Dremio's `ILIKE(expr, pattern)` is a function and takes no escape argument,
// so case folding goes on LOWER(...) and matching on LIKE, which takes one.
// Dremio has no default escape character, so the clause is not optional.
likeIgnoreCase(column, not, param, type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The last commit (e285dad) deleted DremioQueryTemplates.test.ts and DruidQueryTemplates.test.ts and reverted the additions to PinotQueryTemplates.test.ts, so Dremio and Pinot now have no test covering anything this PR changes.

The rationale given — "Druid checks it by values against a real cluster" — holds for Druid, but Druid is the one dialect of the three that has an engine in CI. For the other two the shape test was the only guard, and shape is exactly what the fix consists of:

  • This line is a behavioural rewrite of the SQL emitted for every Dremio LIKE filter (ILIKE(col, …)LOWER(col) LIKE LOWER(…) ESCAPE '\') plus the negation-placement parse-error fix. Dremio has no compose file and is commented out of the integration matrix (push.yml:426), so nothing anywhere executes it. test/unit/params-escaping.test.ts asserts on a hand-written SQL string passed to applyParams — it never calls DremioQuery, so it would not notice this method regressing.
  • Pinot's tesseract.ilike clause is likewise unpinned: the surviving PinotQueryTemplates.test.ts only covers sql_table FROM rendering and LIMIT/OFFSET ordering, and the driver-suite LIKE cases are skipped in fixtures/pinot.json.

A shape assertion is a weak test when an engine is available and the only possible test when one is not. Suggest restoring the two dialects' cases (they were already tightened to pin the whole predicate across four operators on both planners, which is the form that catches the clause drifting and the negation landing back inside the call). Dropping Druid's shape file is fine now that the value test supersedes it.

Also worth updating the PR description — its "How it was verified" and "The tests fail without the fix — they are the first and third commits" sections now describe tests that are no longer in the branch.

Fix this →

Comment thread packages/cubejs-druid-driver/test/druid-driver.test.ts Outdated
The file declared two contradictory things about Druid's LIKE five lines apart:
`expressions.like_escape` was deleted, which is how a dialect says its LIKE
takes no ESCAPE clause - the push-down rewrite is gated on that template being
present - while `tesseract.ilike` emits exactly that clause.

Druid takes the clause. Asked directly, on the datasource the test ingests:

    name LIKE '%a\b%' ESCAPE '\'   ->  a\b
    name LIKE '%a\b%'              ->  (nothing)
    name LIKE 'a%b'  ESCAPE '\'    ->  aXb, a\b

So the deletion was the stale half. Removing it drops a restriction on the SQL
API rather than adding one: LIKE with an escape sequence can now push down to
Druid instead of falling back.

Also covers the escape character itself in the value cases, the third thing
escaped in a user value and the one whose mishandling costs a row rather than
adding one, with `aXb` as the decoy - and reads the ingestion credentials from
the driver config instead of repeating them, so a cluster reached through
TEST_DRUID_HOST cannot leave only the ingestion helper failing to authenticate.

    Tests: 18 passed against a real Druid

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 2m 29s —— View job


Re-review of fa85c00: the like_escape contradiction is resolved correctly and verified by values. 1 low (newly reachable LOWER(pattern) + escape-char mismatch), 1 prior thread still open (Dremio/Pinot have no test).

Full review — click to expand
  • Read the diff since the last round (fa85c00)
  • Re-check prior findings against current code
  • Verify the push-down gate claim against like_expr.rs
  • Resolve stale threads / dedupe (2 resolved, 1 re-affirmed)
  • Post review

Prior findings — status

# Finding Status
DruidQuery.ts deletes expressions.like_escape while emitting the clause Fixed and verified — the delete is gone, and the commit message records the direct broker check (LIKE '%a\b%' ESCAPE '\'a\b; without the clause → nothing). Thread resolved
Druid test hardcodes credentials; LIKE_ROWS lost the backslash value Fixed — reads config.user/config.password, and a\b plus a contains 'a\b' case are back. Thread resolved
Dremio and Pinot have no test covering what this PR changes Still openthread stands unchanged; packages/cubejs-dremio-driver/test/unit/ still holds only params-escaping.test.ts, and PinotQueryTemplates.test.ts still has one sql_table case. Not re-posting

Verified independently this round

  • The gate claim is exactly right. like_expr.rs:90-110 checks expressions/like_escape only inside if escape_char.is_some(), then separately requires expressions/like or expressions/ilike. So the deletion was blocking push-down of LIKE … ESCAPE and nothing else, and removing it cannot make any previously-pushed-down query worse — it only admits queries that used to fall back.
  • The two halves of the file now agree. tesseract.ilike (line 72) and DruidFilter.likeIgnoreCase (line 21) both emit the clause; nothing declares Druid cannot render it. The comment on lines 69-71 says why in three lines, within the repo guidance.
  • The new value case discriminates in the direction claimed. escapeWildcardChars turns a\b into a\\b, so the pattern is %a\\b% with ESCAPE '\' → one literal backslash. Drop the clause and \\ is two literal backslashes, matching nothing — a missing row, which is the failure mode neither % nor _ produces. Good addition.
  • notContains '%' expectation still matches the row set after the two new rows (50Xyoff, off, plain, a\b, aXb — the five without a literal percent).
  • Credentials now flow from one place, so a TEST_DRUID_HOST cluster with different auth fails the driver queries and the ingestion helper alike rather than only the latter.

Small accuracy nit, not worth a thread: the comment calls aXb "the decoy" for the escape-character case, but no plausible mishandling of a\b produces aXb — dropping the escape or the backslash both yield zero rows, not that row. It earns its place in notContains '%'; the decoy framing overstates it.

New finding

Low — packages/cubejs-druid-driver/src/DruidQuery.ts:71 (inline). The push-down this commit unblocks wraps like_escape around expressions.ilike (line 62), which lowercases the pattern but not the escape character like_escape renders. A pushed-down ILIKE '%aXb%' ESCAPE 'X' therefore sends LOWER('%aXb%') — the X becomes x — against a clause still naming X, and the escape silently stops applying. Only affects a non-lowercase escape character, and the same shape exists in every dialect emulating ILIKE with LOWER(...), so it is pre-existing in kind. Flagged because this commit is what makes it reachable for Druid; not a blocker.

Not run

No node_modules in this checkout, so neither the Druid integration suite (needs Docker plus a multi-minute ingestion) nor the driver unit suites were executed here. This round is static review plus the like_expr.rs gate reading; the "18 passed against a real Druid" in the commit message is the behavioural evidence.

· branch [`tesseract-like-pattern-escaping`](https://github.com/cube-js/cube/tree/tesseract-like-pattern-escaping)

templates.tesseract.ilike = 'LOWER({{ expr }}) {% if negated %}NOT {% endif %}LIKE {{ pattern }}';
// Same escape clause as DruidFilter.likeIgnoreCase; it cannot go inside
// `like_pattern`, whose pattern is wrapped in CONCAT(...). Druid accepts the
// clause, so `expressions.like_escape` stays for the SQL API to push down.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Restoring expressions.like_escape is the right call — like_expr.rs:108 only consults it when the query carries an escape character, so this strictly widens push-down, and the value cases in druid-driver.test.ts now settle that Druid parses and honours the clause on a CONCAT(...) pattern. Thread resolved.

Low, and newly reachable because of it: the push-down path it unblocks composes like_escape around expressions.ilike (line 62), which lowercases the patternLOWER(expr) LIKE LOWER(pattern) ESCAPE 'X'. The escape character rendered by like_escape is not lowered, so a pushed-down col ILIKE '%aXb%' ESCAPE 'X' sends a pattern whose X has become x while the clause still names X: the escape stops applying and the predicate matches axb literally instead of ab.

Only bites a non-lowercase escape character, which is unusual (\ is the common one, and the filter path always uses it), and the same shape exists in every dialect that emulates ILIKE with LOWER(...) — so it's pre-existing in kind, not introduced here. Noting it because this commit is what makes it reachable for Druid. Not worth blocking on; if you'd rather not carry it, LOWER({{ pattern }}) could be dropped in favour of leaving the pattern alone, but that changes case-folding semantics and belongs in its own change.

…ter boots

The ingestion the LIKE cases rely on never published in CI, and the wait read
as a bare timeout. The compose file bind-mounts `./storage` as `/opt/data` -
Druid's local deep storage (`druid_storage_storageDirectory=/opt/data/segments`)
and its indexing-log directory. That path is gitignored, so it does not exist at
checkout; Docker then creates the mount source owned by root while the image
runs as `druid`, and the task cannot write either directory.

Only indexing writes there, which is why the query-only cases passed and these
did not, and Docker Desktop makes bind mounts writable whatever the container
user - so the whole thing is invisible off Linux. Confirmed the dependency is
real: a successful run leaves the published segment under `storage/segments/` and
the task log and report under `storage/indexing-logs/`.

The wait also now reports what Druid says about the task instead of only that
rows never arrived, so an ingestion that dies for some other reason names the
reason.

    Tests: 18 passed, cluster booted by the test rather than reused

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 2m 20s —— View job


Re-review of 0a67f6e: the deep-storage diagnosis holds up and I verified each link in it. 2 low (the chmod fails on the very state it repairs; deprecated task-status field + hard-failing poll), 2 prior threads re-affirmed.

Full review — click to expand
  • Read the diff since the last round (0a67f6e)
  • Verify the deep-storage claim against the compose file, environment and .gitignore
  • Re-check prior findings / CI wiring
  • Resolve stale threads, dedupe (nothing newly stale — 7 already resolved, 2 still apply)
  • Post review

The diagnosis checks out, link by link

Every step of the commit message is independently confirmable in the tree, which is unusual for an "it was a permissions thing" fix:

  • packages/cubejs-druid-driver/.gitignore:1 ignores storage, so the bind-mount source genuinely does not exist at checkout.
  • environment:53-56 puts both druid_storage_storageDirectory=/opt/data/segments and druid_indexer_logs_directory=/opt/data/indexing-logs under the mount, and ./storage:/opt/data is mounted on coordinator, historical and middlemanager (docker-compose.yml:39,85,109) — not on broker or router.
  • That split is exactly why the four query-only cases passed while ingestion did not: nothing on the read path touches /opt/data until a segment has to be published there.
  • The Docker Desktop remark is the reason this could only show up in CI, so "works on my machine" was never evidence here.

path.resolve(path.dirname(__filename), '../../') resolves to the package root from dist/test/, so both the compose lookup and the new mkdir are cwd-independent — jest dist/test/*.js invoked from anywhere gets the same directory. And the job is reachable: druid is in the integration matrix (push.yml:424) with use_tesseract_sql_planner: true.

Prior findings — status

# Finding Status
Druid non-literal LIKE pattern / ESCAPE honoured Fixed — value cases against the real cluster, and this commit is what makes them actually run
like_escape deleted while emitting the clause Fixed in fa85c00, thread resolved
Hardcoded credentials; missing backslash row Fixed, thread resolved
Dremio and Pinot have no test for what this PR changes Still openthread stands; cubejs-dremio-driver/test/unit/ still holds only params-escaping.test.ts. Not re-posting
LOWER(pattern) vs. unlowered escape char on push-down Still openthread, low, your call. Not re-posting

Findings

Low — test/druid-driver.test.ts:77. chmodSync throws EPERM on a directory the caller does not own, and a root-owned storage/ left by a pre-fix run is precisely the state this repairs — so on those machines beforeAll now dies with a chmod error instead of ingesting. Guard with an accessSync write check, or drop the bind mount for a named volume (the cleaner shape: Docker seeds ownership from the image, no mkdir, and no world-writable dirs or published segments left in the working tree).

Low — test/druid-driver.test.ts:233. status.status is the deprecated alias for statusCode; present in 27.0.0, but TEST_DRUID_VERSION is a knob, and if it goes away the poll throws ended undefined and misattributes a parse problem to ingestion. Separately, the status probe throws on any non-2xx, so one transient router 503 aborts all 14 cases where the old loop retried — worth failing soft and leaving the deadline as the only hard stop.

Smaller notes, not worth threads

  • The error message improvement is real and well-scoped: is ${last} and … did not become queryable in time distinguishes "task never ran" from "task succeeded, segment not handed off", which is the distinction the 4-minute wait exists for.
  • druidRequest's GET path still sends Content-Type: application/json with no body — harmless, Druid ignores it.
  • taskStatus is polled even after SUCCESS, one extra HTTP call per 2 s tick against a local cluster. Immaterial.
  • The comment at lines 67-72 is six lines, over the repo's three-line guidance, but this is the one place where I would not ask for it shorter: it records a non-reproducible-off-Linux fact that a future reader deleting the mkdir as dead code would otherwise have to rediscover from a CI-only failure.

Not run

No node_modules in this checkout, so the Druid suite (Docker plus a multi-minute ingestion) was not executed here. This round is static verification of the compose/env/gitignore chain; the "18 passed, cluster booted by the test rather than reused" in the commit message is the behavioural evidence, and 18 matches the 4 existing cases plus 7 × 2 planners.

· branch [`tesseract-like-pattern-escaping`](https://github.com/cube-js/cube/tree/tesseract-like-pattern-escaping)

const created = path.join(composePath, 'storage', dir);

fs.mkdirSync(created, { recursive: true });
fs.chmodSync(created, 0o777);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The diagnosis is right and the evidence in the commit message is convincing (storage is in this package's .gitignore:1, environment:53-56 points both druid_storage_storageDirectory and druid_indexer_logs_directory under /opt/data, and only middlemanager writes there — which is exactly why the query-only cases passed).

One wrinkle: chmodSync fails with EPERM on a directory the caller does not own, and the machines that hit this bug are precisely the ones that already have a root-owned packages/cubejs-druid-driver/storage/ left behind by an earlier run. mkdirSync(..., { recursive: true }) is a no-op there, then the chmod throws and beforeAll fails — so the fix breaks on the state it was written to repair, with an error (EPERM: operation not permitted, chmod) that reads as nothing to do with Druid.

Cheapest guard is to skip the chmod when the directory is already writable:

Suggested change
fs.chmodSync(created, 0o777);
fs.mkdirSync(created, { recursive: true });
try {
fs.accessSync(created, fs.constants.W_OK);
} catch {
fs.chmodSync(created, 0o777);
}

That still throws on a root-owned dr-xr-xr-x, but with chmod as the visibly failing step rather than as a silent precondition.

Worth considering the alternative shape too, since this is a test-side workaround for a compose-side fact: turning ./storage into a named volume (like the five *_var volumes already there) lets Docker seed ownership from the image, needs no mkdir, and stops the suite from leaving world-writable directories and published segments in the working tree across runs. The bind mount only earns its keep if someone inspects the segments by hand — which the commit message did, but the test does not.


last = status;

if (status !== 'RUNNING' && status !== 'PENDING' && status !== 'WAITING' && status !== 'SUCCESS') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two small robustness points on the new watch, both about it turning a wait into a hard failure:

  1. status.status is the deprecated alias on Druid's TaskStatusPlus; the canonical field is statusCode. It is present in 27.0.0, which is why the run passed, but TEST_DRUID_VERSION is a knob (docker-compose.yml:36) — on a version that drops the alias, status is undefined, the !== chain is all true, and every LIKE case fails in beforeAll with ended undefined: no error message, which points at the ingestion rather than at the parsing. statusCode ?? status costs nothing and keeps the reason honest.

  2. taskStatus goes through druidRequest, which throws on any non-2xx. With restart: always on the router, one transient 503 during the poll now aborts all 14 cases where the old loop would have retried on the next tick. Since the loop already tolerates the row query failing, letting the status probe fail soft keeps the deadline as the only hard stop:

Suggested change
if (status !== 'RUNNING' && status !== 'PENDING' && status !== 'WAITING' && status !== 'SUCCESS') {
try {
const { statusCode, status, errorMsg } = await taskStatus(task);
last = statusCode ?? status;
if (!['RUNNING', 'PENDING', 'WAITING', 'SUCCESS'].includes(last)) {
throw new Error(`Ingestion task ${task} ended ${last}: ${errorMsg ?? 'no error message'}`);
}
} catch (e) {
if (e instanceof Error && e.message.startsWith('Ingestion task')) {
throw e;
}
}

(taskStatus's return type would need statusCode?: string alongside status.) The startsWith sniff is a bit ugly — a small typed error class, or hoisting the terminal-status check outside the try, reads better if you prefer.

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

Labels

data source driver driver:dremio driver:druid Issues relating to the Druid driver driver:pinot javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant