evalEngine: Implement ELT and FIELD #15249
Conversation
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
requested review from
harshit-gangal,
systay,
frouioui and
GuptaManan100
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
| switch arg.Type { | ||
| case sqltypes.VarBinary, sqltypes.Binary, sqltypes.Blob: | ||
| if tc.Collation != collations.CollationBinaryID { | ||
| c.asm.Convert_xce(offset, arg.Type, tc.Collation) | ||
| } | ||
| case sqltypes.VarChar, sqltypes.Char, sqltypes.Text: | ||
| fromCharset := colldata.Lookup(arg.Col.Collation).Charset() | ||
| toCharset := colldata.Lookup(tc.Collation).Charset() | ||
| if fromCharset != toCharset && !toCharset.IsSuperset(fromCharset) { | ||
| c.asm.Convert_xce(offset, arg.Type, tc.Collation) | ||
| } | ||
| default: | ||
| c.asm.Convert_xce(offset, arg.Type, tc.Collation) | ||
| } |
There was a problem hiding this comment.
Should we create a common function for this? We are now using the same logic in 3 functions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15249 +/- ##
==========================================
- Coverage 67.34% 65.81% -1.54%
==========================================
Files 1560 1561 +1
Lines 192571 195157 +2586
==========================================
- Hits 129695 128435 -1260
- Misses 62876 66722 +3846 ☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
One thing to look at with stuff like this, is how much test runtime we're adding. It might be too much with all these permutations? How long does it take to run these tests?
There was a problem hiding this comment.
about 24 sec, in the comparison test. total 567338 number of tests.
CONCAT_WS takes 12 sec, total 229438 number of tests
There was a problem hiding this comment.
I think we should reduce this then, that's a lot of time for a single subtest here.
There was a problem hiding this comment.
Removed the test for 4 inputs. Total test time reduced to 6 sec, total 56234 number of tests.
Given, |
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
changed the title
| return 1 | ||
| } | ||
|
|
||
| if containsOnlyInt64 { |
There was a problem hiding this comment.
The idea behind the compiler is that we can emit efficient code once types are known. It looks like these are compile time known values, so we should split this up into separate Fn_FIELD functions. Like maybe Fn_FIELD_i, Fn_FIELD_s etc.
Signed-off-by: Noble Mittal <[email protected]>
dbussink
added
Type: Enhancement
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
If there's a literal NULL with only numerical types, we also have to convert to DOUBLE instead of ignoring the NULL. Signed-off-by: Dirkjan Bussink <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]>
Description
This PR adds implementation of
ELTandFIELDfunctions in evalengine.Related Issue(s)
Fixes part of #9647
Checklist
Deployment Notes