Convert string literals to the operand type at prepare time in comparisons - #9108
Open
ChudaykinAlex wants to merge 1 commit into
Open
Convert string literals to the operand type at prepare time in comparisons#9108ChudaykinAlex wants to merge 1 commit into
ChudaykinAlex wants to merge 1 commit into
Conversation
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.
Problem
In a comparison between a typed column and a string literal —
col >= '2018-07-01',col BETWEEN 'lo' AND 'hi'— the literal stays adtype_textnode all the way toexecution. The string→type conversion is then performed once per row, even though
the literal is constant and its target type is known at prepare time.
On a 2 000 000-row full scan this is a measurable share of the total time, and it is
worst for date/time types, where parsing a string into a timestamp is expensive.
Change
ComparativeBoolNode::dsqlPass()now converts a string literal operand into a typedconstant once, at prepare time, using the descriptor of the opposite operand as the
target type.
MAKE_constant_from_literal(LiteralNode*, const dsc*)insrc/dsql/make.cpp.arg1vsarg2,arg2vsarg1, and — forblr_between— toarg3(the upper bound) vs
arg1.DOUBLE,DECFLOAT(16),DECFLOAT(34),INT128,DATE,TIME,TIME WITH TIME ZONE,TIMESTAMP,TIMESTAMP WITH TIME ZONE.Anything else is left alone.
Why this is safe
Two guards keep the optimization on exactly the path the runtime would have taken
anyway.
1. Only when the runtime would convert in the same direction.
The conversion is performed only when the reference operand has a strictly higher
comparison priority — i.e. the same coercion
CVT2_comparewould apply per row. Ifthe comparison is textual (string vs string, or string vs a lower-priority type), the
node is left untouched. This is what keeps
'2000-01-01' = '2000-01-01 'behaving asa string comparison rather than silently becoming a date comparison.
2. Only for
dtype_textoperands.The helper returns early unless the literal is
dtype_text, so nothing happens foralready-typed literals, parameters, or expressions.
Regressions found during review and fixed in this branch
Both were caught by the existing QA suite and are fixed in the commits below.
1 = -:idfailed with "Invalid data type for negation" (core_5395).The first iteration forced
DsqlDescMaker::fromNode()on the opposite operand forany literal, including numeric ones. For
1 = -:idthat means asking for thedescriptor of a negation over a not-yet-known parameter, which throws. Since the
optimization only ever applies to
dtype_text, thedtype_textcheck was movedbefore the descriptor is requested, so the reference operand is only inspected
when the literal is actually a string.
Relative date/time verbs were converted at prepare time (core_0053, core_1173, core_5421).
'NOW','TODAY','TOMORROW'and'YESTERDAY'are resolved at execution againstthe current clock. Converting them during prepare both failed and produced a value
frozen at prepare time.
MAKE_constant_from_literal()now detects these four verbs(case-insensitive, surrounding blanks ignored) and declines to convert when the target
is a date/time type, leaving them to the runtime.
Testing
test_correctness.sql
Correctness. A dedicated script compares the optimized form against the canonical
one —
col <op> 'literal'vscol <op> CAST('literal' AS <type>)— over 100-rowdeterministic data sets: 47 assertions covering
>=,=,<>, literal-on-the-left,BETWEENandNOT BETWEEN, acrossDATE,TIME,TIME WITH TIME ZONE,TIMESTAMP,TIMESTAMP WITH TIME ZONE,DOUBLE,DECFLOAT(16),DECFLOAT(34),INT128, plus the relative verbs and literal-vs-literal forms.All 47 assertions pass, and produce identical values on a build with the
optimization disabled and on the patched build — the change does not alter results,
only where the conversion happens.
Regression suite. core_5395, core_0053, core_1173 and core_5421 pass.
Performance
perf_compare.sql
2 000 000 rows per table, warmed page cache, full scan,
COUNT(*)over a range,isqlwithSET STATS ON. Two builds, identical except for this change, run against the same script and the same data:DATETIMESTAMPDECFLOAT(16)INT128Row counts and fetch counts are identical between the two builds for every query.
Date/time parsing benefits most, numeric parsing least — as expected, since turning a
string into a timestamp costs far more than turning it into an integer.
BETWEENupper bound (arg3),DATE, same data — converting the upper bound inaddition to the lower one is worth roughly a further 2.3× on this query.
Note for reviewers
Converting at prepare time moves the failure point for a malformed literal: something
like
WHERE date_col >= 'not a date'would raise its conversion error while preparingthe statement rather than while executing it. This follows from the design and is the
intended trade-off, but it is a user-visible change in when the error appears, so it
is worth an explicit decision. I have not re-verified the exact error text and timing
on the current build.