feat(udf): support date, time, timestamp and interval type #10399
Conversation
xxchan
left a comment
There was a problem hiding this comment.
Just did a quick browse and have some questions:
- The diff looks like changing the implementation (e.g., nano -> micro), instead of adding new types. Could you explain a bit what's the behavior before and how it's changed? (If it's rejected, where is the boundary?)
- There are also some minor changes regarding Decimal. Could you explain what's changed? (Moving that into a separate PR is even more preferable)
| DataType::Decimal => Self::Decimal128(28, 0), // arrow precision can not be 0 | ||
| DataType::Decimal => Self::Decimal128(38, 0), // arrow precision can not be 0 |
There was a problem hiding this comment.
What does this change mean?
There was a problem hiding this comment.
Decimal128 has a max precision of 38 digits, but RisingWave only supports 28 digits due to its dependency on rust-decimal. Given that we have a plan to support 38 digits in the near future, it seems better to define the parameterless DECIMAL as 38 digits in the UDF protocol.
| /** | ||
| * Combination of Period and Duration. | ||
| */ | ||
| public class PeriodDuration extends org.apache.arrow.vector.PeriodDuration { |
There was a problem hiding this comment.
Could you elaborate on the motivation of not using org.apache.arrow.vector.PeriodDuration directly? Maybe it can be part of the javadoc of this class.
There was a problem hiding this comment.
I wanted to re-export this class in our package so that users don't need to be aware of arrow. But it seems that Java doesn't support a way like in Rust.
java/udf/src/test/java/com/risingwave/functions/TestUdfServer.java
Outdated
Show resolved
Hide resolved
c3e44b4 to
31be48c
Compare
183183d to
8f07ff2
Compare
Codecov Report
@@ Coverage Diff @@
## main #10399 +/- ##
=======================================
Coverage 70.47% 70.47%
=======================================
Files 1261 1261
Lines 215173 215182 +9
=======================================
+ Hits 151650 151657 +7
- Misses 63523 63525 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Previously the arrow date/time types were arbitrary chosen and were not tested. This PR simply fixes the implementation. We use microsecond as the unit because RisingWave only supports microsecond resolution. |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR adds date/time types support for both Java UDF and Python UDF.
Checklist
./risedev check(or alias,./risedev c)Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note
Date, time, timestamp and interval type are now supported in UDF.