[SQL] Cast of a VARIANT to string should work for most variant types#6044
[SQL] Cast of a VARIANT to string should work for most variant types#6044mihaibudiu merged 1 commit intofeldera:mainfrom
Conversation
mythical-fred
left a comment
mythical-fred
left a comment
There was a problem hiding this comment.
The runtime behavior change here spans more scalar variant families than the new test covers; see inline.
There was a problem hiding this comment.
This match now defines CAST(variant AS VARCHAR) semantics for every scalar branch here, but the new regression only exercises a subset of them (INT, VARCHAR, REAL/DOUBLE, BOOLEAN, UUID, and one short interval). Please add coverage for the other newly-supported scalar families too -- at least DECIMAL, DATE, TIME, TIMESTAMP, LONG INTERVAL, and VARBINARY, plus one negative case such as ARRAY/MAP still failing. This codepath is all hand-written formatting logic, so the untested branches are exactly where drift will hide.
|
|
||
| #[doc(hidden)] | ||
| const fn sign(negative: bool) -> &'static str { | ||
| pub(crate) const fn sign(negative: bool) -> &'static str { |
There was a problem hiding this comment.
you probably dont need doc(hidden) on pub(crate)things since it won't get exposed
| let sign = sign(months < 0); | ||
| let years = months.unsigned_abs() / 12; | ||
| let months = months.unsigned_abs() % 12; | ||
| Ok(SqlString::from(format!("{sign}{years}-{months:02}"))) |
There was a problem hiding this comment.
maybe whatever type of x is should have a .into() for sqlstring
There was a problem hiding this comment.
does clippy not complain about this? format!("{}", x) seems like somehting it would have a problem with
it would be nice to optimize this, it will do two allocations (to make astring then to make a SqlString
There was a problem hiding this comment.
There may be a third string allocation, if this is called from a cast, which may truncate the string after producing it.
You mean, optimize the Boolean case or all cases?
mihaibudiu
force-pushed
the
issue5938
branch
2 times, most recently
from
9534e12 to
aef5b24
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
mihaibudiu
force-pushed
the
issue5938
branch
3 times, most recently
from
89d2565 to
9d59d11
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
Fixes #5938
Describe Manual Test Plan
Implemented conversion of VARIANT to CHAR types. Previously this conversion would return NULL for all values except Variant::String values. Now it will behave roughly as a CAST of the underlying value to the CHAR type.
E.g.
CAST(CAST(TRUE AS VARIANT) AS VARCHAR)used to returnNULLand now will returntrue.Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes
See first paragraph above and changelog.