-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: update to substrait 0.69.0 #345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I did have one question about mapping PrecisionTime into Spark.
override def visit(expr: Type.PrecisionTime): DataType = { | ||
Util.assertMicroseconds(expr.precision()) | ||
TimestampType | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
- From what I'm seeing, Spark doesn't have a direct representation for Time types. Do the semantics of TimestampType match the Substrait expecations for Time? There is no existing mapping for the old
Type.Time
either. - Assuming 1, does the PrecisionTime type have a timezone? Should this be
TimestampNTZType
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know TBH... I've taken out the mapping entirely so that it will default to throwing an exception (like the old Type.Time
).
This can be implemented in the future, once it's better understood. I only intended to do a minimal implementation of this new type in order to fix the build error caused by pulling in the latest substrait library :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken out the mapping entirely
That works for me, especially as it wasn't already handled on the deprecated path.
Required to fix the build errors when upgrading to substrait-0.69.0 protos
8b8513a
to
0d7fefb
Compare
Required to pick up the decimal round() function definition