alamb commented on code in PR #16170:
URL: https://github.com/apache/datafusion/pull/16170#discussion_r2132891294
##########
datafusion/expr/src/expr.rs:
##########
@@ -274,16 +275,16 @@ use sqlparser::ast::{
/// assert!(rewritten.transformed);
/// // to 42 = 5 AND b = 6
/// assert_eq!(rewritten.data, lit(42).eq(lit(5)).and(col("b").eq(lit(6))));
-#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
+#[derive(Clone, PartialEq, PartialOrd, Eq, Debug, Hash)]
pub enum Expr {
/// An expression with a specific name.
Alias(Alias),
/// A named reference to a qualified field in a schema.
Column(Column),
/// A named reference to a variable in a registry.
ScalarVariable(DataType, Vec<String>),
- /// A constant value.
- Literal(ScalarValue),
+ /// A constant value along with associated metadata
+ Literal(ScalarValue, Option<BTreeMap<String, String>>),
Review Comment:
I recommend:
1. Adding a comment about why BTree is used to capture the context of this PR
2. Change the metadata to be Arc'd so that `Clone`ing literals with metadata
is not expensive.
I don't think this is a blocker for this PR, but do think it is important
for the long term. Thus if we merge this API in for 48.0.0 I think we should be
prepared to make a breaking API change for 49.0.0
I will try and whip up a prototype of what I am talking about
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
