[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3890: skip failing tests default to false

[GitBox <gi...@apache.org>]

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


##########
datafusion/core/src/config.rs:
##########
@@ -237,6 +237,15 @@ impl BuiltInConfigs {
                  to reduce the number of rows decoded.",
                 false,
             ),
+            #[cfg(test)]
+            ConfigDefinition::new_bool(

Review Comment:
   ```suggestion
               // skip_failing_rules default to true in tests so that regressions are caught much sooner
               ConfigDefinition::new_bool(
   ```



##########
datafusion/core/src/config.rs:
##########
@@ -237,6 +237,15 @@ impl BuiltInConfigs {
                  to reduce the number of rows decoded.",
                 false,
             ),
+            #[cfg(test)]
+            ConfigDefinition::new_bool(
+                OPT_OPTIMIZER_SKIP_FAILED_RULES,
+                "When set to true, the logical plan optimizer will produce warning \
+                messages if any optimization rules produce errors and then proceed to the next \
+                rule. When set to false, any rules that produce errors will cause the query to fail.",

Review Comment:
   ```suggestion
                   "OVERRIDDEN FOR TEST",
   ```



##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1972,6 +1972,7 @@ mod tests {
         Ok(())
     }
 
+    #[ignore]

Review Comment:
   We should have a ticket to track this failure. How does it fail with the change in default?



##########
datafusion/core/src/config.rs:
##########
@@ -237,6 +237,15 @@ impl BuiltInConfigs {
                  to reduce the number of rows decoded.",
                 false,
             ),
+            #[cfg(test)]
+            ConfigDefinition::new_bool(
+                OPT_OPTIMIZER_SKIP_FAILED_RULES,
+                "When set to true, the logical plan optimizer will produce warning \
+                messages if any optimization rules produce errors and then proceed to the next \
+                rule. When set to false, any rules that produce errors will cause the query to fail.",

Review Comment:
   I think this could be very confusing to people (that tests and defaults are different) so I suggest some more comments to make it clearer



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