You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Omega359 (via GitHub)" <gi...@apache.org> on 2024/01/29 03:58:58 UTC
[PR] Add a make_date function #9024 [arrow-datafusion]
Omega359 opened a new pull request, #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040
## Which issue does this PR close?
Closes #9024
## Are these changes tested?
Yes, in code and sql logic tests.
## Are there any user-facing changes?
A new make_date function is available, documented in user guide with examples in the datafusion-examples module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470329669
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
It's this section in columnar_values_to_array (and the similar bit in make_scalar_function_with_hints)
```
let args = args
.iter()
.map(|arg| arg.clone().into_array(inferred_length))
.collect::<Result<Vec<_>>>()?;
```
The into_array method seems to eventually percolate down to code like this in primitive_array.rs
```
/// Creates a PrimitiveArray based on a constant value with `count` elements
pub fn from_value(value: T::Native, count: usize) -> Self {
unsafe {
let val_buf = Buffer::from_trusted_len_iter((0..count).map(|_| value));
Self::new(val_buf.into(), None)
}
}
```
It was that copy of values into the array that I was attempting to work around .. which I guess I did at the cost of a cast for every element in the array. That bit of code is what I was thinking might have been better handled as a single value array. In any case I'll think about this some more and see what I can do to optimize this a bit more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1473176455
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
done, #9086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470271635
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
I think you can use `columnar_values_to_array` for this, which was recently added by @viirya for just this purpose
https://github.com/apache/arrow-datafusion/blob/851bc7de850d0261202b18641561c8bd09abe588/datafusion-examples/examples/simple_udf.rs#L74
##########
datafusion/sqllogictest/test_files/timestamps.slt:
##########
@@ -2233,4 +2233,168 @@ SELECT val, ts1 - ts2 AS ts_diff FROM table_a ORDER BY ts2 - ts1
1 0 days -1 hours 0 mins 0.000000000 secs
2 0 days -1 hours 0 mins 0.000000000 secs
+##########
+## make date tests
+##########
+
+query D
+select make_date(2024, 1, 27);
+----
+2024-01-27
+
+query D
+select make_date(42, 1, 27);
+----
+0042-01-27
+
+query D
+select make_date(99, 1, 27);
+----
+0099-01-27
+
+query D
+select make_date(2024, 2, 29);
+----
+2024-02-29
+
+query D
+select make_date(10001, 1, 27);
+----
++10001-01-27
+
+query D
+select make_date('2024', '01', '27');
+----
+2024-01-27
+
+query D
+select make_date(12 + 2012, '01', '27');
+----
+2024-01-27
+
+query D
+select make_date(2024::bigint, 01::bigint, 27::bigint);
+----
+2024-01-27
+
+query D
+select make_date(arrow_cast(2024, 'Int64'), arrow_cast(1, 'Int64'), arrow_cast(27, 'Int64'));
+----
+2024-01-27
+
+query D
+select make_date(arrow_cast(2024, 'Int32'), arrow_cast(1, 'Int32'), arrow_cast(27, 'Int32'));
+----
+2024-01-27
+
+statement ok
+create table table_nums (year int, month int, day int) as values
+ (2024, 1, 23),
+ (2023, 11, 30);
+
+query D
+select make_date(t.year, t.month, t.day) from table_nums t;
+----
+2024-01-23
+2023-11-30
+
+query D
+select make_date(2021, t.month, t.day) from table_nums t;
+----
+2021-01-23
+2021-11-30
+
+query D
+select make_date(t.year, 3, t.day) from table_nums t;
+----
+2024-03-23
+2023-03-30
+
+query D
+select make_date(t.year, t.month, 4) from table_nums t;
+----
+2024-01-04
+2023-11-04
+
+query D
+select make_date('2021', t.month, t.day) from table_nums t;
+----
+2021-01-23
+2021-11-30
+
+query D
+select make_date(t.year, '3', t.day) from table_nums t;
+----
+2024-03-23
+2023-03-30
+
+query D
+select make_date(t.year, t.month, '4') from table_nums t;
+----
+2024-01-04
+2023-11-04
+
+statement ok
+insert into table_nums values (2024, null, 23);
+
+query error DataFusion error: Execution error: Unable to parse date from 2024, 0, 23
+select make_date(t.year, t.month, t.day) from table_nums t;
+
+statement ok
+drop table table_nums;
+
+statement ok
+create table table_strings (year varchar(4), month varchar(2), day varchar(2)) as values
+ ('2024', '1', '23'),
+ ('2023', '11', '30');
+
+query D
+select make_date(t.year, t.month, t.day) from table_strings t;
+----
+2024-01-23
+2023-11-30
+
+statement ok
+insert into table_strings values (2024, null, 23);
+
+query error DataFusion error: Execution error: Unable to parse date from 2024, 0, 23
+select make_date(t.year, t.month, t.day) from table_strings t;
+
+statement ok
+drop table table_strings;
+
+query error DataFusion error: Execution error: Unable to parse date from 2024, 13, 23
Review Comment:
😍 -- 💯 for test coverage
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
+ // as any scalar will need to be converted to an array of len `len`.
+ let len = args
+ .iter()
+ .fold(Option::<usize>::None, |acc, arg| match arg {
+ ColumnarValue::Scalar(_) => acc,
+ ColumnarValue::Array(a) => Some(a.len()),
+ });
+
+ let is_scalar = len.is_none();
+ let array_size = if is_scalar { 1 } else { len.unwrap() };
+
+ let years = cast_column(&args[0], &DataType::Int32, None)?;
+ let months = cast_column(&args[1], &DataType::Int32, None)?;
+ let days = cast_column(&args[2], &DataType::Int32, None)?;
+
+ let value_fn = |col: &ColumnarValue, pos: usize| -> Result<i32> {
+ match col {
+ ColumnarValue::Array(a) => Ok(a.as_primitive::<Int32Type>().value(pos)),
+ ColumnarValue::Scalar(s) => match s {
+ ScalarValue::Int32(i) => match i {
+ Some(i) => Ok(*i),
+ None => {
+ exec_err!("Unable to parse date from null/empty value")
+ }
+ },
+ _ => unreachable!(),
+ },
+ }
+ };
+
+ // since the epoch for the date32 datatype is the unix epoch
+ // we need to subtract the unix epoch from the current date
+ // note this can result in a negative value
+ let unix_days_from_ce = NaiveDate::from_ymd_opt(1970, 1, 1)
+ .unwrap()
+ .num_days_from_ce();
+
+ let mut builder: PrimitiveBuilder<Date32Type> = PrimitiveArray::builder(array_size);
+
+ for i in 0..array_size {
+ let y = value_fn(&years, i)?;
+ let m = u32::try_from(value_fn(&months, i)?);
+ let d = u32::try_from(value_fn(&days, i)?);
+
+ if m.is_err() {
+ return exec_err!(
+ "Month value '{:?}' is out of range",
+ value_fn(&months, i).unwrap()
+ );
+ }
Review Comment:
I think you can write this logic like so to avoid the unwraps below:
```suggestion
let Ok(m) = m else {
return exec_err!(
"Month value '{:?}' is out of range",
value_fn(&months, i).unwrap()
);
};
```
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
+ // as any scalar will need to be converted to an array of len `len`.
+ let len = args
+ .iter()
+ .fold(Option::<usize>::None, |acc, arg| match arg {
+ ColumnarValue::Scalar(_) => acc,
+ ColumnarValue::Array(a) => Some(a.len()),
+ });
+
+ let is_scalar = len.is_none();
+ let array_size = if is_scalar { 1 } else { len.unwrap() };
+
+ let years = cast_column(&args[0], &DataType::Int32, None)?;
+ let months = cast_column(&args[1], &DataType::Int32, None)?;
+ let days = cast_column(&args[2], &DataType::Int32, None)?;
+
+ let value_fn = |col: &ColumnarValue, pos: usize| -> Result<i32> {
Review Comment:
This could probably be implemented more efficiently with some sort of iterator that wrapped a `ColumnarValue` and returned i32s -- that way you could iterate over all three arrays with `zip` and not have to do the downcast eah time, etc
-- and then you could generate specialized implementations for ColumnarValue::Array and ColumnarValue::Scalar
However, given `make_date` is likely to be run with all constants I don't think there is any pressing need to optimize the implementation unless someone has a particular need for a faster implementation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470289270
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
I did start with an iterator approach however the creation of the arrays for scalar values just didn't seem like the best approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470315212
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
In fact I'm curious why arrow didn't just go with a SingleValueArray vs the creation of a buffer that it currently does. Perhaps it made some operations too complex?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1473136555
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
> If you think these benchmarks are useful I'll submit a pull request.
Thank you @Omega359 !
I think they would be useful (in particular, I think with the benchmarks we could then write up a ticket about "optimize performance of make_date" and I suspect we might find someone who is interested in that sort of thing who wants to contribute
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470319653
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
Review Comment:
You can represent the idea now using RunEndEncoded array (with the count of size) but that is relatively new and not well supported yet in arrow-rs or DataFusion
Arrow-rs also now has https://docs.rs/arrow/latest/arrow/array/struct.Scalar.html which is similar to the `ColumnarValue` API
If it had a SingleValueArray that would likely have to be handled specially in all the kernels, but perhaps I don't fully understand what you are suggesting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a make_date function #9024 [arrow-datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #9040:
URL: https://github.com/apache/arrow-datafusion/pull/9040#discussion_r1470302230
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -497,6 +502,99 @@ pub fn make_current_time(
move |_arg| Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(nano)))
}
+/// make_date(year, month, date) SQL function implementation
+pub fn make_date(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ if args.len() != 3 {
+ return exec_err!(
+ "make_date function requires 3 arguments, got {}",
+ args.len()
+ );
+ }
+
+ // first, identify if any of the arguments is an Array. If yes, store its `len`,
+ // as any scalar will need to be converted to an array of len `len`.
+ let len = args
+ .iter()
+ .fold(Option::<usize>::None, |acc, arg| match arg {
+ ColumnarValue::Scalar(_) => acc,
+ ColumnarValue::Array(a) => Some(a.len()),
+ });
+
+ let is_scalar = len.is_none();
+ let array_size = if is_scalar { 1 } else { len.unwrap() };
+
+ let years = cast_column(&args[0], &DataType::Int32, None)?;
+ let months = cast_column(&args[1], &DataType::Int32, None)?;
+ let days = cast_column(&args[2], &DataType::Int32, None)?;
+
+ let value_fn = |col: &ColumnarValue, pos: usize| -> Result<i32> {
Review Comment:
Initially I was tempted to try adding an IntoIterator for ColumnarValue that accepted an optional size (for scalar) which returns an option of either the array value at the current pos or the scalar value. That seemed a bit much given my current Rust skill level though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org