-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support to unparse ScalarValue::IntervalMonthDayNano
to String
#10956
Support to unparse ScalarValue::IntervalMonthDayNano
to String
#10956
Conversation
3de86df
to
9ae676c
Compare
datafusion/sql/src/unparser/expr.rs
Outdated
@@ -859,6 +878,35 @@ impl Unparser<'_> { | |||
} | |||
} | |||
|
|||
fn process_interval_nanosecond(nano: i64) -> String { |
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.
There seems to be similar code already in https://github.com/apache/arrow-rs/blob/72467c670f8c38130e4743347407f1a542e59e0c/arrow-cast/src/display.rs#L600-L649
I think we could potentially use it via
https://docs.rs/arrow/latest/arrow/util/display/fn.array_value_to_string.html
After first converting ScalarValue to a one value array: https://docs.rs/datafusion/latest/datafusion/common/enum.ScalarValue.html#method.to_array
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.
Thanks @alamb. It looks great. I'll address this today. :)
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.
@alamb After using ArrayFormatter, I noticed that the results contain many redundant words. For example:
input expr: interval_month_day_nano_lit("-3 MONTH"),
output result: r#"INTERVAL '0 YEARS -3 MONS 0 DAYS 0 HOURS 0 MINS 0.000000000 SECS'"#,
---
input expr: interval_month_day_nano_lit("1 YEAR 1 MONTH 1 DAY 3 HOUR 10 MINUTE 20 SECOND"),
output result: r#"INTERVAL '0 YEARS 13 MONS 1 DAYS 3 HOURS 10 MINS 20.000000000 SECS'"#,
I think both of these are valid SQL. They just don't look very smart, but it's okay with me.
I implemented the code like
ScalarValue::IntervalMonthDayNano(Some(_i)) => {
let wrap_array = v.to_array()?;
let Some(result) = array_value_to_string(&wrap_array, 0).ok() else {
return internal_err!("Unable to convert IntervalMonthDayNano to string");
};
let interval = Interval {
value: Box::new(ast::Expr::Value(SingleQuotedString(result.to_uppercase()))),
leading_field: None,
leading_precision: None,
last_field: None,
fractional_seconds_precision: None,
};
Ok(ast::Expr::Interval(interval))
}
What do you think? Does it make sense?
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.
Hmm... OK. I think it would be an enhancement for the ArrowFormatter. If we want to make it smarter, I think we should modify the behavior in the arrow-cast
crate.
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.
Hmm... OK. I think it would be an enhancement for the ArrowFormatter. If we want to make it smarter, I think we should modify the behavior in the
arrow-cast
crate.
I agree -- I filed apache/arrow-rs#5914 to track this suggestion
( | ||
interval_month_day_nano_lit( | ||
"1 YEAR 1 MONTH 1 DAY 3 HOUR 10 MINUTE 20 SECOND", | ||
), | ||
r#"INTERVAL '0 YEARS 13 MONS 1 DAYS 3 HOURS 10 MINS 20.000000000 SECS'"#, | ||
), |
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.
I removed some tests because I think we use the formatter in arrow-cast
now. We can assume the formatting behaviors are tested there. We don't need to keep so many tests here.
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.
Thanks again @goldmedal !
datafusion/sql/src/unparser/expr.rs
Outdated
@@ -859,6 +878,35 @@ impl Unparser<'_> { | |||
} | |||
} | |||
|
|||
fn process_interval_nanosecond(nano: i64) -> String { |
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.
Hmm... OK. I think it would be an enhancement for the ArrowFormatter. If we want to make it smarter, I think we should modify the behavior in the
arrow-cast
crate.
I agree -- I filed apache/arrow-rs#5914 to track this suggestion
Thanks @alamb |
…che#10956) * support to unparse ScalarValue::IntervalMonthDayNano to String * use array formatter to format the interval string
…che#10956) * support to unparse ScalarValue::IntervalMonthDayNano to String * use array formatter to format the interval string
…che#10956) * support to unparse ScalarValue::IntervalMonthDayNano to String * use array formatter to format the interval string
…che#10956) * support to unparse ScalarValue::IntervalMonthDayNano to String * use array formatter to format the interval string
Which issue does this PR close?
Closes #10795 .
Rationale for this change
I handle only the nanosecond, millisecond, second, minute, and hour units for human readability based on the nanosecond value.
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
no