Repository navigation
fix(sql): a predicate over a function-wrapped column dropped the function - #372
Merged
Merged
Conversation
…tion
`Expression.painless` compared the Painless PARAMETER NAME whenever the operand
was registered on the context. That is the whole operand only where the function
chain folds into the parameter's own declaration as a method — `YEAR(d)` becomes
`def param1 = doc['d']....get(ChronoField.YEAR)`. A chain that renders as an
EXPRESSION cannot fold, so the parameter held the untransformed doc-value and the
function VANISHED: `WHERE WEEKDAY(d) = 0` emitted
def param1 = (doc['d'].size() == 0 ? null : doc['d'].value...);
param1 == null ? false : (param1 == 0)
a `ZonedDateTime` against an `int`. Painless does not reject that, it evaluates
it as FALSE — so `=` and `IN` answered ZERO rows with HTTP 200 and `NOT IN`
answered EVERY row, while `>` / `BETWEEN` / a string `compareTo` failed the
shard. The SELECT list was correct all along, which is why it survived: a
`script_field` renders the operand itself and never enters this method.
The population is every function whose Painless is not a suffix method chain,
and it is wider than the six the issue names: WEEKDAY/DAYOFWEEK, LAST_DAY,
DATE_FORMAT, DATETIME_FORMAT, DATE_PARSE, DATETIME_PARSE with their aliases,
plus every CAST/CONVERT/TRY_CAST/SAFE_CAST and ISNULL/ISNOTNULL — 283 statements
of a 4,209-predicate census.
Eight repairs, each forced by a measurement on real Elasticsearch:
1. the parameter shortcut is taken only when the parameter IS the chain rendering
2. the coercion reads the type the chain RENDERS (new `Identifier.chainType`),
not the column's — the schema-resolved `WEEKDAY(d)` claimed to be a TIMESTAMP
3. a TEMPORAL chain over a TEMPORAL column keeps the column's Java type; coercing
it emitted `.atStartOfDay(...)`, a `LocalDate` method, onto a `ZonedDateTime`
4. numeric-vs-numeric comparisons skip the widening: `((long) <guarded ternary>)`
is a compile error, and Painless compares boxed numerics itself
5. a safe cast's `try`/`catch` is hoisted into the script prologue in a QUERY
context, so it can be an operand at all; the PERSISTED processor/transform
renderings keep their bytes
6. `IN` over a numeric operand compares with `==`: `List.contains` is Java
`equals`, which is false across boxed types, so any non-Integer answered
nothing
7. `COALESCE` at one argument emitted `String.valueOf( : param1))` and at three or
more an unbalanced expression Elasticsearch cannot parse, in every venue
8. the `.toLocalDate()` narrowing no longer lands on a non-temporal column, and
fires on both sides of a comparison or on neither
The RIGHT operand had the same lie: `check` chose the comparison spelling from
the right-hand identifier's `out`, i.e. its column's type, so `YEAR(d) =
YEAR(ts)` compared two `int`s with `isEqual` and `CAST(num AS INT) = CAST(num2 AS
INT)` stringified the left and lost every match. One derivation, `valueType`,
now answers for the right what `chainType` answers for the left.
Two guards, and they measure different things. `PredicateTransformSurvivalSpec`
generates its population from `SQLKeywords.functionWords` and asks three
questions with no allow-list: is the transform still there, is every method the
emission calls declared on the receiver the pipeline hands it (walking the chain
and re-typing at each step, the JDK deciding), and do the delimiters balance.
`PredicateFunctionResultSpec` EXECUTES the predicates over a real index and
asserts ROW SETS, because comparing a `ZonedDateTime` to an `int` is not an error
but a wrong answer, and no byte pin can see that.
Closes #367
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ME_FORMAT
On 6.8 a `date` doc-value reaches Painless as a `JodaCompatibleZonedDateTime`
while `DateTimeFormatter.format` requires a `java.time.TemporalAccessor`, and the
operand of these two functions is passed as an ARGUMENT, so it never reaches the
UTC normalisation the CHAINED date functions get. The query is refused with a
script error there and works on 7, 8 and 9.
MEASURED on real 6.8.23 over `created_at = 2025-01-10`, which narrows the claim
considerably — only a BARE date column fails:
DATE_FORMAT('2025-01-10'::DATE, '%Y-%m-%d') works
DATE_FORMAT(CAST(created_at AS DATE), '%Y-%m-%d') works
DATE_FORMAT(DATE_TRUNC(created_at, DAY), '%Y-%m-%d') works
DATE_FORMAT(created_at, '%Y-%m-%d') refused
So the documentation carries a one-edit workaround rather than a bare warning,
and records that every formatting example already published uses a cast and is
therefore unaffected. The note goes in the function reference (in the same shape
the `STDDEV`/`VARIANCE` version limit already uses), in the known-limitations
page, and in the REPL help corpus for both functions.
⚠️ This documents the limit instead of fixing it, deliberately. The one-line
normalisation that fixes the bare-column case was written, measured and REVERTED:
it breaks five shapes whose operand is already a `LocalDate` (`DATE_TRUNC`,
`DATE_ADD`, `CAST(d AS DATE)`, `CURRENT_DATE`, `LAST_DAY`), since `LocalDate` has
no `toInstant()`. Instrumenting the call site found why: the narrowing is already
REQUESTED — `identifier.painlessMethods` holds
`.toInstant().atZone(Z).toLocalDate()` — and never reaches the declaration, which
renders as the bare doc-value, because `PainlessParam` identity is the doc-value
STRING and the mutated object is not the rendered one. That is the same defect as
issue #370, and one fix in how a parameter's declaration is rendered serves both.
Also adds `EmissionCostProbe`, the companion of `ParseCostProbe` for the other
half of what a request pays before Elasticsearch sees it: schema resolution and
script emission. Same protocol, same refusal to assert a timing.
Refs #371, #370
Co-Authored-By: Claude Opus 5 (1M context) <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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What was wrong
Expression.painlesscompared the Painless parameter name whenever the operand was registered onthe context. That is the whole operand only where the function chain folds into the parameter's own
declaration as a method —
YEAR(d)becomesdef param1 = doc['d']….get(ChronoField.YEAR). A chainthat renders as an expression cannot fold, so the parameter held the untransformed doc-value and
the function vanished:
A
ZonedDateTimeagainst anint. Painless does not reject that — it evaluates it as false,which is precisely the reported symptom:
=andINanswered zero rows with HTTP 200,NOT INanswered every row, and
>/BETWEEN/ a stringcompareTofailed the shard. TheSELECTlist wascorrect all along because a
script_fieldrenders the operand itself and never enters this method.🔴 The issue's own diagnosis was wrong, and that is worth recording. It reported the malformed
statement-sequence Painless as "not the cause" and the obvious fix as "ruled out" — both conclusions
came from
painless(None), a rendering path production never takes for aWHERE. The review of #366that reported "compares a
ZonedDateTimeto an integer" was right.Population — derived, not listed
A census over every function spelling the grammar accepts × operand shapes × right-hand sides
convicts 283 statements: the six date functions the issue names, plus every
CAST/CONVERT/TRY_CAST/SAFE_CASTandISNULL/ISNOTNULL.The eight repairs, each forced by a measurement
Identifier.chainType)WEEKDAY(d)claimedTIMESTAMP, so the extractedintwas converted as a dateWHERE LAST_DAY(d) = ts→dynamic method [java.time.ZonedDateTime, atStartOfDay/1] not found, where it had WORKED before((long) <guarded ternary>)→script_exception: compile errortry/catchis hoisted into the prologue, QUERY context onlydef left1 = try { … };→ compile error; persisted processor/transform bytes unchangedINover a numeric operand compares with==[9,12].contains(<Long>)and<Double>matched NOTHING;<Integer>matched —List.containsis JavaequalsCOALESCErenders balanced at every arityString.valueOf( : param1)); ≥3 args →no_viable_alt_exception, in EVERY venue including a plainSELECT.toLocalDate()narrowing respects the column's type, and fires on both sides or neither.toLocalDate()on a keyword column failed the shard; narrowing one side gaveCannot cast java.time.ZonedDateTime to java.time.chrono.ChronoLocalDateBoth operands, not one.
checkchose the comparison spelling from the right-hand identifier'sout— its column's type — soYEAR(d) = YEAR(ts)compared twoints withisEqual(
dynamic method [java.lang.Integer, isEqual/1] not found) andCAST(num AS INT) = CAST(num2 AS INT)stringified the left and lost every match, silently. One derivation,
valueType, now answers for theright what
chainTypeanswers for the left.Verification
Two guards that measure different things.
PredicateTransformSurvivalSpec(sql, unit) generates its population fromSQLKeywords.functionWords— 18,273 predicates — and asks three questions, no allow-list:
other sees: a signature diff misses
DATE_PARSE, whoseLocalDate.parsealso appears on the right,and
ISNULL, whose transform contains no call at all);method CHAIN and re-typing at each step from the JDK's own return types;
PredicateFunctionResultSpec(testkit, 5 clients) executes the predicates over a real index andasserts ROW SETS, because comparing a
ZonedDateTimeto anintis not an error but a wrong answer,and no byte pin can see that.
Results
Set()for= 0, all seven rows forNOT IN).live defect until the mutation proved it fires.
(
headerCheck scalafmtSbtCheck scalafmtCheck test:scalafmtCheck), Scala 2.12 compiles.Blast radius outside predicates: 36 of 2,211 statements, over SELECT script fields, sorts,
buckets, date math, transform and ingest renderings — 24
COALESCE(every one an emissionElasticsearch previously rejected) and 12
TRY_CAST/SAFE_CAST(the query-context hoist). Thepersisted ingest renderings that change are the three broken
COALESCEones; noTRY_CASTprocessoror transform byte moves.
Release notes
built on the old answers (a saved filter, a dashboard, a materialized view whose WHERE used one of
these functions) changes behaviour.
INover a numeric operand emits<param> == v1 || <param> == v2instead of[v1,v2].contains(…).TRY_CAST/SAFE_CASTin a query render a hoistedtry/catch;COALESCErenders balanced atevery arity. Downstream fixtures pinning generated Painless need updating.
Identifier.chainType,PainlessContext.bindLocalWith, and two protectedmembers on
Expression. No existing signature changed, so thesqlmodule staysbinary-compatible.
assume-skipped and theverbatim Elasticsearch errors recorded in the spec.
Found and NOT fixed, each recorded in the spec with its measurement
DATE_TRUNC(d, MONTH) < dis always false,DATE_ADD(d, INTERVAL 1 DAY) = dalways true. Presentbefore and after this change; repairing it means changing Painless parameter identity, which
reaches every emission that reuses one.
JodaCompatibleZonedDateTime, soDATE_FORMAT/DATETIME_FORMATcannot work there in any venue, nor can a temporal expression compared with a rawdate column. Half of it is one measured line; it moves bytes another story pins as
"byte-identical to before the fix", so it is left to its own change.
Documentation — the Elasticsearch 6.8 limit of
DATE_FORMAT/DATETIME_FORMAT(#371)A second commit documents the limit rather than fixing it, and the measurement narrows the claim a
long way: on 6.8 only a bare date column fails. Measured on real 6.8.23 over
created_at = 2025-01-10:DATE_FORMAT('2025-01-10'::DATE, '%Y-%m-%d')— every published exampleDATE_FORMAT(CAST(created_at AS DATE), '%Y-%m-%d')DATE_FORMAT(DATE_TRUNC(created_at, DAY), '%Y-%m-%d')DATE_FORMAT(created_at, '%Y-%m-%d')So the docs give a one-edit workaround, not just a warning, and record that no published example is
affected. The note lands in the function reference (the shape
STDDEV/VARIANCE's version limitalready uses), in the known-limitations page, and in the REPL help corpus for both functions.
repairs the bare-column case and breaks five shapes whose operand is already a
LocalDate(
DATE_TRUNC,DATE_ADD,CAST(d AS DATE),CURRENT_DATE,LAST_DAY) —LocalDatehas notoInstant(). Instrumenting the call site found the real mechanism: the narrowing is alreadyREQUESTED (
identifier.painlessMethodsholds.toInstant().atZone(Z).toLocalDate()) and never reachesthe parameter declaration, because
PainlessParamidentity is the doc-value STRING, so the objectwhose method list is mutated is not the object the prologue renders. That is #370's defect, and one
fix in parameter-declaration rendering serves both.
This commit also adds
EmissionCostProbe, the companion ofParseCostProbefor the other half of whata request pays before Elasticsearch sees it — schema resolution and script emission. Same protocol,
and it asserts no timing, for the reason #269/#270 recorded.
Closes #367
🤖 Generated with Claude Code