feat: [comet-parquet-exec] Schema adapter fixes #1139
Conversation
| } | ||
| DataType::Timestamp(TimeUnit::Millisecond, None) => { | ||
| let array = as_primitive_array::<TimestampMillisecondType>(&array); | ||
| let tz: Tz = tz.parse()?; |
There was a problem hiding this comment.
Is this called frequently (per row)? timezone parse is somewhat expensive (and does not change for a session).
There was a problem hiding this comment.
This is once per array, but I think the parsing could happen once during planning rather than per batch/array.
There was a problem hiding this comment.
Makes sense, we can defer this for the moment.
| /// The schema of the table. This is the expected schema after conversion | ||
| /// and it should match the schema of the query result. | ||
| projected_table_schema: SchemaRef, | ||
| required_schema: SchemaRef, |
native/spark-expr/src/cast.rs
Outdated
| allow_incompat, | ||
| )?), | ||
| _ if is_datafusion_spark_compatible(from_type, to_type, allow_incompat) => { | ||
| _ if ugly_hack_for_poc |
There was a problem hiding this comment.
What are the cases (that we know of) where this gets invoked? (If we know we can replace this flag with an explicit check for those cases, perhaps?)
There was a problem hiding this comment.
I believe that we need to implement specific logic for adapting parquet schemas, rather than re-using our Spark-compatible cast. There is likely some overlap, so we can refactor the common code out. For example, regular spark casts do not need to support unsigned integers, but we need this when adapting Parquet schemas.
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?