findepi commented on code in PR #13466:
URL: https://github.com/apache/datafusion/pull/13466#discussion_r1846439004


##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -223,6 +233,30 @@ impl ScalarUDFImpl for DatePartFunc {
     }
 }
 
+fn is_integar_part(part: &str) -> bool {
+    let part = part_normalization(part);
+    matches!(
+        part.to_lowercase().as_str(),
+        "year"
+            | "month"
+            | "week"
+            | "day"
+            | "hour"
+            | "minute"
+            | "qtr"
+            | "quarter"
+            | "doy"
+            | "dow"

Review Comment:
   second, millisecond, micro, nanos too?
   
   



##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -223,6 +233,30 @@ impl ScalarUDFImpl for DatePartFunc {
     }
 }
 
+fn is_integar_part(part: &str) -> bool {
+    let part = part_normalization(part);
+    matches!(
+        part.to_lowercase().as_str(),
+        "year"
+            | "month"
+            | "week"
+            | "day"
+            | "hour"
+            | "minute"
+            | "qtr"
+            | "quarter"
+            | "doy"
+            | "dow"
+    )
+}
+
+// Try to remove quote if exist, if the quote is invalid, return original 
string and let the downstream function handle the error
+fn part_normalization(part: &str) -> &str {
+    part.strip_prefix(|c| c == '\'' || c == '\"')
+        .and_then(|s| s.strip_suffix(|c| c == '\'' || c == '\"'))

Review Comment:
   
   why would we want to support unmatched quotes like `'..."`?
   
   the date_part argument shouldn't contain any quotes, why would we want to 
support quotes at all?
   
   
   i see this is pre-existing, so okay to keep, but still wonder why we're 
doing this, doesn't feel right



##########
datafusion/sqllogictest/test_files/expr.slt:
##########
@@ -1487,32 +1487,32 @@ SELECT extract(epoch from arrow_cast('1969-12-31', 
'Date64'))
 
 # test_extract_interval
 
-query R
+query I
 SELECT extract(year from arrow_cast('10 years', 'Interval(YearMonth)'))
 ----
 10
 
-query R
+query I

Review Comment:
   Should we have explicit tests inspecting `arrow_typeof(extract(....))`?



-- 
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-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to