-
Notifications
You must be signed in to change notification settings - Fork 23
[TASK-167] Support Arrow append operations for decimals, temporal types #206
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
base: main
Are you sure you want to change the base?
[TASK-167] Support Arrow append operations for decimals, temporal types #206
Conversation
4fdd56c to
e8de061
Compare
|
@leekeiabstraction @luoyuxia PTAL 🙏 |
leekeiabstraction
left a comment
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.
Thank you for the PR! Left some comments.
|
@leekeiabstraction Thank you for the review. |
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.
Pull request overview
This PR adds support for temporal types (Date, Time, Timestamp NTZ/LTZ) and Decimal types to the Python bindings' append() method. It enables users to write these types directly from Python datetime objects and decimal.Decimal to Fluss tables.
Changes:
- Added Arrow-to-Fluss type mapping with precision-aware conversions for Time32/Time64 and Timestamp types
- Implemented Python-to-Datum converters for datetime.date, datetime.time, datetime.datetime (both naive and timezone-aware), pandas.Timestamp, and decimal.Decimal
- Enhanced
write_pandas()to pass expected schema for proper type casting - Updated example to demonstrate new type support
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bindings/python/src/utils.rs | Added precision-aware Arrow-to-Fluss type mappings for Time32/Time64 and Timestamp types, with timezone-based routing for TimestampLtz vs Timestamp NTZ |
| bindings/python/src/table.rs | Implemented comprehensive type converters for temporal and decimal types, including time conversion constants, caching mechanisms, and helper functions for Python type conversions |
| bindings/python/example/example.py | Updated example to demonstrate Date, Time, Timestamp, and Decimal type usage with both Arrow and row-level append operations |
| bindings/python/Cargo.toml | Added bigdecimal 0.4 dependency for Decimal type support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Total milliseconds (note: days can be negative for dates before epoch) | ||
| let total_micros = days * MICROS_PER_DAY + seconds * MICROS_PER_SECOND + microseconds; | ||
| let millis = total_micros / MICROS_PER_MILLI; | ||
| let nano_of_milli = ((total_micros % MICROS_PER_MILLI) * MICROS_PER_MILLI) as i32; |
Copilot
AI
Jan 24, 2026
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.
The constant MICROS_PER_MILLI is being used as both the modulo divisor and as a conversion factor from microseconds to nanoseconds. While numerically correct (both are 1000), this is semantically confusing. Consider using a more appropriately named constant for the conversion factor, such as NANOS_PER_MICRO, or use a literal 1000 for clarity. This would improve code readability and make the intent clearer.
| hour as i8, | ||
| minute as i8, | ||
| second as i8, | ||
| microsecond as i32 * 1000, |
Copilot
AI
Jan 24, 2026
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.
For consistency with the rest of the code that uses named constants for time conversions (lines 29-35), this magic number should be replaced with a named constant. Consider using the existing NANOS_PER_MILLI constant divided by MICROS_PER_MILLI, or defining a new constant NANOS_PER_MICRO = 1000.
Linked issue: close #167
Add Date, Time, Timestamp (NTZ/LTZ), and Decimal type support to Python bindings' append() method.
Brief change log