[logical-types] use Scalar in Expr::Logical #12793
Conversation
github-actions
bot
added
sql
| pub struct Scalar { | ||
| value: ScalarValue, | ||
| pub value: ScalarValue, |
There was a problem hiding this comment.
I've discovered just now that it's possible to match inside a partially public struct. We could consider removing the value and into_value method and just use the field.
Not all matches were update to support pattern-matching this field (I've discovered this feature in the middle of this changes). They can be updated subsequently.
notfilippo
force-pushed
the
fr/scalar-in-expr
branch
from
30ecb7a
to
88c75fc
Compare
|
|
||
| impl fmt::Debug for Scalar { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| self.value.fmt(f) |
There was a problem hiding this comment.
Please keep derived Debug. It reports both value & type, which might be useful for debugging.
There was a problem hiding this comment.
This is me being lazy since some tests were failing because they compare debug prints of plans. I can revert this, it was added to not introduce other changes.
| @@ -116,6 +143,10 @@ impl Scalar { | |||
| ScalarValue::iter_to_array_of_type(scalars.map(|scalar| scalar.value), &data_type) | |||
| } | |||
|
|
|||
| pub fn cast_to(&self, data_type: &DataType) -> Result<Self> { | |||
There was a problem hiding this comment.
Do we need this on the Scalar itself?
Once we logicalize it, this function will drag us. Maybe let's have a utlity method somewhere else.
There was a problem hiding this comment.
I think this function is useful to keep here because we can then enforce the same constraints introduced in the construction of the struct and potentially support logic like
data_type.is_logically_equivalent_to(self.data_type) => self.data_type = data_type (skipping the cast)
| let utf8_val = if utf8_val == "foo" { | ||
| "bar".to_string() | ||
| } else { | ||
| utf8_val | ||
| }; |
There was a problem hiding this comment.
nit: avoid formatting changes unless needed
There was a problem hiding this comment.
Weird. I just ran cargo fmt. 🤷
| @@ -41,39 +42,45 @@ pub trait TimestampLiteral { | |||
| fn lit_timestamp_nano(&self) -> Expr; | |||
| } | |||
|
|
|||
| impl From<ScalarValue> for Expr { | |||
| fn from(value: ScalarValue) -> Self { | |||
| Self::Literal(Scalar::from(value)) | |||
There was a problem hiding this comment.
at some point we should rename Literal to Constant. constant is what this represents, while literal is a syntax-related term.
|
Let's keep this code moving on to the branch Thank you for your review @findepi |
Item for #12622. (See plan described in the EPIC for context around this change)