[GitHub] [arrow-datafusion] jackwener commented on a diff in pull request #5132: [BugFix] abort plan if order by column not in select list

["jackwener (via GitHub)" <gi...@apache.org>]

jackwener commented on code in PR #5132:
URL: https://github.com/apache/arrow-datafusion/pull/5132#discussion_r1099714549


##########
datafusion/core/tests/dataframe.rs:
##########
@@ -124,6 +125,39 @@ async fn sort_on_unprojected_columns() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test]

Review Comment:
   I suggest to add those sql in comment into `sqllogicaltest`.
   In this test, we can `data-derive-test`



##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -403,6 +403,28 @@ impl LogicalPlanBuilder {
                 Ok(())
             })?;
 
+        // if current plan is distinct or current plan is repartition and its child plan is distinct,
+        // then this plan is a select distinct plan
+        let is_select_distinct = match self.plan {
+            LogicalPlan::Distinct(_) => true,
+            LogicalPlan::Repartition(Repartition { ref input, .. }) => {
+                matches!(input.as_ref(), &LogicalPlan::Distinct(_))
+            }
+            _ => false,
+        };
+
+        // for select distinct, order by expressions must appear in select list

Review Comment:
   ```suggestion
           // for select distinct, order by expressions must exist in select list
   ```



##########
datafusion/core/tests/dataframe.rs:
##########
@@ -124,6 +125,39 @@ async fn sort_on_unprojected_columns() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test]

Review Comment:
   I also suggest add test in `datafusion/sql/tests/integration_test.rs`.
   In this test, we can see the `Plantree` of plan to check those sql
   



-- 
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