jayzhan211 commented on code in PR #12536: URL: https://github.com/apache/datafusion/pull/12536#discussion_r1779319730
########## datafusion/expr-common/src/columnar_value.rs: ########## @@ -89,7 +91,7 @@ pub enum ColumnarValue { /// Array of values Array(ArrayRef), /// A single value - Scalar(ScalarValue), + Scalar(Scalar), Review Comment: > > we can get the LogicalType from it if each DataType has at most one LogicalType > > We should not assume this to be the case. I suppose #12644 may convey why, but even if we don't do Extension Types, this assumption will be very limiting. I'm still not convinced why my assumption is limited. For extension types, we could customize/extend the **logical type** for each `DataType`. It doesn't make sense to have more than 1 logical type that decoded to the same physical type, they are *equivalent* thus should be the same logical type. Even if they have historical issue thus require more than one type for backward compatibility, they are able to do it if they built their type on top of Datafusion's Logical Type. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org