Skip to content
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

Minor: return NULL for range and generate_series #10275

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

Lordworms
Copy link
Contributor

Which issue does this PR close?

Closes #10269

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 29, 2024
@Lordworms Lordworms changed the title return NULL for range and generate_series Minor: return NULL for range and generate_series Apr 29, 2024
@Lordworms Lordworms marked this pull request as ready for review April 29, 2024 00:19
@@ -57,6 +56,7 @@ impl Range {
TypeSignature::Exact(vec![Int64, Int64]),
TypeSignature::Exact(vec![Int64, Int64, Int64]),
TypeSignature::Exact(vec![Date32, Date32, Interval(MonthDayNano)]),
TypeSignature::Any(3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we need to find a new kind of signature that accepts either one exact type or null 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have a new Signature. Maybe I can add more signatures in the following PR along with dealing #10200

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of something like

        TypeSignature::ExactOrNull(valid_types) => {
            current_types.iter().map(|t| {
                if t != &DataType::Null {
                    valid_types.clone()
                } else {
                    vec![DataType::Null]
                }
            })
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be fine. I'll add a ticket to record it

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Lordworms and @jayzhan211 -- looks good to me

FWIW I also verified that returning NULL is consistent with DuckDB:

D select generate_series(DATE '1992-09-01', NULL, INTERVAL '1' YEAR);
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ generate_series(CAST('1992-09-01' AS DATE), NULL, to_years(CAST(trunc(CAST('1' AS DOUBLE)) AS INTEGER))) │
│                                                  int32                                                   │
├──────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│                                                                                                          │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────┘

datafusion/sqllogictest/test_files/array.slt Outdated Show resolved Hide resolved
datafusion/sqllogictest/test_files/array.slt Outdated Show resolved Hide resolved
@@ -166,146 +166,6 @@ impl ScalarUDFImpl for StringToArray {
}
}

make_udf_function!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just an accidental left over copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there might be some other left things

}
}

make_udf_function!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise this looks like an accidental copy too -- here is the other copy:

make_udf_function!(
GenSeries,
gen_series,
start stop step,
"create a list of values in the range between start and stop, include upper bound",
gen_series_udf
);
#[derive(Debug)]
pub(super) struct GenSeries {
signature: Signature,
aliases: Vec<String>,
}
impl GenSeries {
pub fn new() -> Self {
Self {
signature: Signature::one_of(
vec![
TypeSignature::Exact(vec![Int64]),
TypeSignature::Exact(vec![Int64, Int64]),
TypeSignature::Exact(vec![Int64, Int64, Int64]),
TypeSignature::Exact(vec![Date32, Date32, Interval(MonthDayNano)]),
],
Volatility::Immutable,
),
aliases: vec![String::from("generate_series")],
}
}
}
impl ScalarUDFImpl for GenSeries {
fn as_any(&self) -> &dyn Any {
self
}
fn name(&self) -> &str {
"generate_series"
}
fn signature(&self) -> &Signature {
&self.signature
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
Ok(List(Arc::new(Field::new(
"item",
arg_types[0].clone(),
true,
))))
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
match args[0].data_type() {
Int64 => make_scalar_function(|args| gen_range_inner(args, true))(args),
Date32 => make_scalar_function(|args| gen_range_date(args, true))(args),
_ => {
exec_err!("unsupported type for range")
}
}
}
fn aliases(&self) -> &[String] {
&self.aliases
}
}

@jayzhan211 jayzhan211 merged commit dd56837 into apache:main Apr 29, 2024
23 checks passed
@jayzhan211
Copy link
Contributor

thanks @Lordworms and @alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle nulls for range and generate_series
3 participants