notfilippo commented on code in PR #12853:
URL: https://github.com/apache/datafusion/pull/12853#discussion_r1795618569


##########
datafusion/common/src/types/logical.rs:
##########
@@ -0,0 +1,41 @@
+use core::fmt;
+use std::{cmp::Ordering, hash::Hash, sync::Arc};
+
+use super::NativeType;
+
+/// A reference counted [`LogicalType`]
+pub type LogicalTypeRef = Arc<dyn LogicalType>;
+
+pub trait LogicalType: fmt::Debug {
+    fn native(&self) -> &NativeType;
+    fn name(&self) -> Option<&str>;

Review Comment:
   As @findepi proposed 
(https://github.com/apache/datafusion/issues/11513#issuecomment-2401814374) 
maybe a more structured approach is needed.



##########
datafusion/common/src/types/logical.rs:
##########
@@ -0,0 +1,41 @@
+use core::fmt;
+use std::{cmp::Ordering, hash::Hash, sync::Arc};
+
+use super::NativeType;
+
+/// A reference counted [`LogicalType`]
+pub type LogicalTypeRef = Arc<dyn LogicalType>;
+
+pub trait LogicalType: fmt::Debug {
+    fn native(&self) -> &NativeType;
+    fn name(&self) -> Option<&str>;
+}
+
+impl PartialEq for dyn LogicalType {
+    fn eq(&self, other: &Self) -> bool {
+        self.native().eq(other.native()) && self.name().eq(&other.name())
+    }
+}
+
+impl Eq for dyn LogicalType {}
+
+impl PartialOrd for dyn LogicalType {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for dyn LogicalType {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.name()
+            .cmp(&other.name())
+            .then(self.native().cmp(other.native()))
+    }
+}
+
+impl Hash for dyn LogicalType {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.name().hash(state);
+        self.native().hash(state);
+    }
+}

Review Comment:
   Should we implement these traits manually or should we leave to the 
implementors?



##########
datafusion/common/src/types/native.rs:
##########
@@ -0,0 +1,237 @@
+use std::sync::Arc;
+
+use arrow_schema::{DataType, IntervalUnit, TimeUnit};
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub enum NativeType {
+    /// Null type
+    Null,
+    /// A boolean datatype representing the values `true` and `false`.
+    Boolean,
+    /// A signed 8-bit integer.
+    Int8,
+    /// A signed 16-bit integer.
+    Int16,
+    /// A signed 32-bit integer.
+    Int32,
+    /// A signed 64-bit integer.
+    Int64,
+    /// An unsigned 8-bit integer.
+    UInt8,
+    /// An unsigned 16-bit integer.
+    UInt16,
+    /// An unsigned 32-bit integer.
+    UInt32,
+    /// An unsigned 64-bit integer.
+    UInt64,
+    /// A 16-bit floating point number.
+    Float16,
+    /// A 32-bit floating point number.
+    Float32,
+    /// A 64-bit floating point number.
+    Float64,
+    /// A timestamp with an optional timezone.
+    ///
+    /// Time is measured as a Unix epoch, counting the seconds from
+    /// 00:00:00.000 on 1 January 1970, excluding leap seconds,
+    /// as a signed 64-bit integer.
+    ///
+    /// The time zone is a string indicating the name of a time zone, one of:
+    ///
+    /// * As used in the Olson time zone database (the "tz database" or
+    ///   "tzdata"), such as "America/New_York"
+    /// * An absolute time zone offset of the form +XX:XX or -XX:XX, such as 
+07:30
+    ///
+    /// Timestamps with a non-empty timezone
+    /// ------------------------------------
+    ///
+    /// If a Timestamp column has a non-empty timezone value, its epoch is
+    /// 1970-01-01 00:00:00 (January 1st 1970, midnight) in the *UTC* timezone
+    /// (the Unix epoch), regardless of the Timestamp's own timezone.
+    ///
+    /// Therefore, timestamp values with a non-empty timezone correspond to
+    /// physical points in time together with some additional information about
+    /// how the data was obtained and/or how to display it (the timezone).
+    ///
+    ///   For example, the timestamp value 0 with the timezone string 
"Europe/Paris"
+    ///   corresponds to "January 1st 1970, 00h00" in the UTC timezone, but the
+    ///   application may prefer to display it as "January 1st 1970, 01h00" in
+    ///   the Europe/Paris timezone (which is the same physical point in time).
+    ///
+    /// One consequence is that timestamp values with a non-empty timezone
+    /// can be compared and ordered directly, since they all share the same
+    /// well-known point of reference (the Unix epoch).
+    ///
+    /// Timestamps with an unset / empty timezone
+    /// -----------------------------------------
+    ///
+    /// If a Timestamp column has no timezone value, its epoch is
+    /// 1970-01-01 00:00:00 (January 1st 1970, midnight) in an *unknown* 
timezone.
+    ///
+    /// Therefore, timestamp values without a timezone cannot be meaningfully
+    /// interpreted as physical points in time, but only as calendar / clock
+    /// indications ("wall clock time") in an unspecified timezone.
+    ///
+    ///   For example, the timestamp value 0 with an empty timezone string
+    ///   corresponds to "January 1st 1970, 00h00" in an unknown timezone: 
there
+    ///   is not enough information to interpret it as a well-defined physical
+    ///   point in time.
+    ///
+    /// One consequence is that timestamp values without a timezone cannot
+    /// be reliably compared or ordered, since they may have different points 
of
+    /// reference.  In particular, it is *not* possible to interpret an unset
+    /// or empty timezone as the same as "UTC".
+    ///
+    /// Conversion between timezones
+    /// ----------------------------
+    ///
+    /// If a Timestamp column has a non-empty timezone, changing the timezone
+    /// to a different non-empty value is a metadata-only operation:
+    /// the timestamp values need not change as their point of reference 
remains
+    /// the same (the Unix epoch).
+    ///
+    /// However, if a Timestamp column has no timezone value, changing it to a
+    /// non-empty value requires to think about the desired semantics.
+    /// One possibility is to assume that the original timestamp values are
+    /// relative to the epoch of the timezone being set; timestamp values 
should
+    /// then adjusted to the Unix epoch (for example, changing the timezone 
from
+    /// empty to "Europe/Paris" would require converting the timestamp values
+    /// from "Europe/Paris" to "UTC", which seems counter-intuitive but is
+    /// nevertheless correct).
+    ///
+    /// ```
+    /// # use arrow_schema::{DataType, TimeUnit};
+    /// DataType::Timestamp(TimeUnit::Second, None);
+    /// DataType::Timestamp(TimeUnit::Second, Some("literal".into()));
+    /// DataType::Timestamp(TimeUnit::Second, 
Some("string".to_string().into()));
+    /// ```
+    Timestamp(TimeUnit, Option<Arc<str>>),
+    /// A signed 32-bit date representing the elapsed time since UNIX epoch 
(1970-01-01)
+    /// in days.
+    Date,
+    /// A signed 32-bit time representing the elapsed time since midnight in 
the unit of `TimeUnit`.
+    /// Must be either seconds or milliseconds.
+    Time32(TimeUnit),
+    /// A signed 64-bit time representing the elapsed time since midnight in 
the unit of `TimeUnit`.
+    /// Must be either microseconds or nanoseconds.
+    Time64(TimeUnit),
+    /// Measure of elapsed time in either seconds, milliseconds, microseconds 
or nanoseconds.
+    Duration(TimeUnit),
+    /// A "calendar" interval which models types that don't necessarily
+    /// have a precise duration without the context of a base timestamp (e.g.
+    /// days can differ in length during day light savings time transitions).
+    Interval(IntervalUnit),
+    /// Opaque binary data of variable length.
+    Binary,
+    /// Opaque binary data of fixed size.
+    /// Enum parameter specifies the number of bytes per value.
+    FixedSizeBinary(i32),
+    /// A variable-length string in Unicode with UTF-8 encoding.
+    Utf8,
+    /// A list of some logical data type with variable length.
+    List(Box<NativeType>),
+    /// A list of some logical data type with fixed length.
+    FixedSizeList(Box<NativeType>, i32),
+    /// A nested datatype that contains a number of sub-fields.
+    Struct(Box<[(String, NativeType)]>),

Review Comment:
   Should we track nullability of the variants that in `DataType` use the 
`Field` object (data_type + nullability + metadata + other) ?



-- 
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

Reply via email to