You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/15 11:25:03 UTC
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4188: Disallow duplicate interval types during parsing
alamb commented on code in PR #4188:
URL: https://github.com/apache/arrow-datafusion/pull/4188#discussion_r1022669455
##########
datafusion/common/src/parsers.rs:
##########
@@ -42,56 +82,72 @@ pub fn parse_interval(leading_field: &str, value: &str) -> Result<ScalarValue> {
(month_part as i32, day_part as i32, milles_part)
};
- let calculate_from_part =
- |interval_period_str: &str, interval_type: &str| -> Result<(i32, i32, f32)> {
- // @todo It's better to use Decimal in order to protect rounding errors
- // Wait https://github.com/apache/arrow/pull/9232
- let interval_period = match f32::from_str(interval_period_str) {
- Ok(n) => n,
- Err(_) => {
- return Err(DataFusionError::NotImplemented(format!(
- "Unsupported Interval Expression with value {:?}",
- value
- )));
- }
- };
-
- if interval_period > (i32::MAX as f32) {
+ let mut used_interval_types = 0;
+
+ let mut calculate_from_part = |interval_period_str: &str,
+ interval_type: &str|
+ -> Result<(i32, i32, f32)> {
+ // @todo It's better to use Decimal in order to protect rounding errors
+ // Wait https://github.com/apache/arrow/pull/9232
+ let interval_period = match f32::from_str(interval_period_str) {
+ Ok(n) => n,
+ Err(_) => {
return Err(DataFusionError::NotImplemented(format!(
- "Interval field value out of range: {:?}",
+ "Unsupported Interval Expression with value {:?}",
value
)));
}
+ };
- match interval_type.to_lowercase().as_str() {
- "century" | "centuries" => {
- Ok(align_interval_parts(interval_period * 1200_f32, 0.0, 0.0))
- }
- "decade" | "decades" => {
- Ok(align_interval_parts(interval_period * 120_f32, 0.0, 0.0))
- }
- "year" | "years" => {
- Ok(align_interval_parts(interval_period * 12_f32, 0.0, 0.0))
- }
- "month" | "months" => Ok(align_interval_parts(interval_period, 0.0, 0.0)),
- "week" | "weeks" => {
- Ok(align_interval_parts(0.0, interval_period * 7_f32, 0.0))
- }
- "day" | "days" => Ok(align_interval_parts(0.0, interval_period, 0.0)),
- "hour" | "hours" => {
- Ok((0, 0, interval_period * SECONDS_PER_HOUR * MILLIS_PER_SECOND))
- }
- "minute" | "minutes" => {
- Ok((0, 0, interval_period * 60_f32 * MILLIS_PER_SECOND))
- }
- "second" | "seconds" => Ok((0, 0, interval_period * MILLIS_PER_SECOND)),
- "millisecond" | "milliseconds" => Ok((0, 0, interval_period)),
- _ => Err(DataFusionError::NotImplemented(format!(
- "Invalid input syntax for type interval: {:?}",
- value
- ))),
+ if interval_period > (i32::MAX as f32) {
+ return Err(DataFusionError::NotImplemented(format!(
+ "Interval field value out of range: {:?}",
+ value
+ )));
+ }
+
+ let it = IntervalType::from_str(interval_type).map_err(|_| {
+ DataFusionError::NotImplemented(format!(
+ "Invalid input syntax for type interval: {:?}",
+ value
+ ))
+ })?;
+
+ // Disallow duplicate interval types
+ if used_interval_types & (it as u16) != 0 {
+ return Err(DataFusionError::SQL(ParserError::ParserError(format!(
+ "Invalid input syntax for type interval: {:?}",
+ value
+ ))));
Review Comment:
```suggestion
// Disallow duplicate interval types
if used_interval_types & (it as u16) != 0 {
return Err(DataFusionError::SQL(ParserError::ParserError(format!(
"Invalid input syntax for type interval: {:?}. Repeated type {}",
value, interval_type
))));
```
--
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