alamb commented on code in PR #6595:
URL: https://github.com/apache/arrow-datafusion/pull/6595#discussion_r1225777088


##########
datafusion/common/src/dfschema.rs:
##########
@@ -384,8 +384,7 @@ impl DFSchema {
         let self_fields = self.fields().iter();
         let other_fields = other.fields().iter();
         self_fields.zip(other_fields).all(|(f1, f2)| {
-            f1.qualifier() == f2.qualifier()
-                && f1.name() == f2.name()
+            f1.qualified_name() == f2.qualified_name()

Review Comment:
   I am not sure if it matters, but I believe that `qualified_name` creates a 
new `String` where the previous version avoids that allocation.



##########
datafusion/core/tests/sqllogictests/test_files/array.slt:
##########
@@ -110,10 +111,10 @@ select array_position(['h', 'e', 'l', 'l', 'o'], 'l', 4), 
array_position([1, 2,
 4 5 2
 
 # array_positions scalar function
-query III
+query error DataFusion error: SQL error: ParserError\("Expected an SQL 
statement, found: caused"\)

Review Comment:
   these errors look not quite right -- I think there is something wrong with 
sqllogictest `--complete` with multi-line errors 🤔 



##########
datafusion/core/tests/sql/group_by.rs:
##########
@@ -18,6 +18,8 @@
 use super::*;
 
 #[tokio::test]
+#[ignore]
+// TODO: issue: https://github.com/apache/arrow-datafusion/issues/6623

Review Comment:
   Here is a proposed fix: https://github.com/apache/arrow-datafusion/pull/6632



##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -206,7 +206,16 @@ impl<'a> TreeNodeRewriter for ConstEvaluator<'a> {
 
     fn mutate(&mut self, expr: Expr) -> Result<Expr> {
         match self.can_evaluate.pop() {
-            Some(true) => Ok(Expr::Literal(self.evaluate_to_scalar(expr)?)),
+            Some(true) => {
+                // After simplifying the expression, data_type may change, so 
we need to cast it.

Review Comment:
   This seems wrong to me -- the type of the expression should always be the 
same before/after simplification. If it isn't the same that is a bug (perhaps 
we could raise an internal error here?)



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

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

Reply via email to