-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat!: initial support for timestamp #12
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.
Looks mostly good to me.
This should be marked as a breaking change since it will require an update of upstream crates.
crates/proof-of-sql/src/base/database/record_batch_dataframe_conversion.rs
Show resolved
Hide resolved
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.
Can we add some tests in query_expr_tests.rs
and integration_tests.rs
as well?
# Rationale for this change Incorporates a small amount of feedback from #12 # What changes are included in this PR? Corrects naming in tests and removes unsupported feature in group_by_util # Are these changes tested? yes
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Rationale for this change
Provides the initial typing support for a postgresql-like
TimeStamp
type. The proofsTimeStampTZ
is typed over a customTimeUnit
andTimeZone
type.The choice is made to type out TimeStamp in this way because the arrow::datatypes::TimeStamp has a required TimeUnit field and an Option TimeZone field. Typing out our own TimeStamp with these fields gives us greater control over the arrow type if we want. We can also jut default to seconds and UTC if needed, which would simplify this design a bit.
Typing Rationale
The
arrow::datatypes::timezone
is typed over aTimeUnit
and an optional timezoneOption<Arc<str>>
. Thus in our application it makes sense to have a mapping of this metadata:example:
If this becomes burdensome, we could just as easily remove the timezone type and simply default to UTC, and handle any timezone conversion in DML and DDL. We will align with postgresql and store all times as UTC by default.
Finally, the
PoSQLTimeUnit
type gives us the flexibility to store times in either seconds, milliseconds, nanoseconds, or microseconds for high precision. This type maps directly toTimeUnit
which we alias asArrowTimeUnit
in this PR.What changes are included in this PR?
Typing updates:
impl ArrayRefExt for ArrayRef -> to_curve_25519_scalar & to_column
impl<S: Scalar> FromIterator<i64> for OwnedColumn<S>
impl<CP: CommitmentEvaluationProof> DataAccessor<CP::Scalar> for OwnedTableTestAccessor<CP>
Are these changes tested?
Tests:
Split: