You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2025/08/10 06:04:49 UTC
[PR] Set a lower threshold for clippy to flag large error variants [datafusion]
Jefffrey opened a new pull request, #17109:
URL: https://github.com/apache/datafusion/pull/17109
Closes #16652
Most of the work to close this issue was done by #16653 and #16672 anyway; here we just try set a lower threshold for clippy to warn us about error variants being too large (from default of 128 to 70) so hopefully we can identify any future issues quicker.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Set a lower threshold for clippy to flag large error variants [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #17109:
URL: https://github.com/apache/datafusion/pull/17109#discussion_r2265133603
##########
datafusion/sqllogictest/src/engines/datafusion_engine/error.rs:
##########
@@ -28,7 +28,7 @@ pub type Result<T, E = DFSqlLogicTestError> = std::result::Result<T, E>;
pub enum DFSqlLogicTestError {
/// Error from sqllogictest-rs
#[error("SqlLogicTest error(from sqllogictest-rs crate): {0}")]
- SqlLogicTest(#[from] TestError),
+ SqlLogicTest(#[from] Box<TestError>),
Review Comment:
I don't think this matters too much since it's sqllogictest stuff?
##########
clippy.toml:
##########
@@ -9,4 +9,14 @@ disallowed-types = [
# Lowering the threshold to help prevent stack overflows (default is 16384)
# See: https://rust-lang.github.io/rust-clippy/master/index.html#/large_futures
-future-size-threshold = 10000
\ No newline at end of file
+future-size-threshold = 10000
+
+# Be more aware of large error variants which can impact the "happy path" due
+# to large stack footprint when considering async state machines (default is 128).
+#
+# Value of 70 picked arbitrarily as something less than 100.
+#
+# See:
+# - https://github.com/apache/datafusion/issues/16652
+# - https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
+large-error-threshold = 70
Review Comment:
Initially I picked 64, but there were some error variants that were _exactly_ 64 and got flagged, hence increased to 70
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Set a lower threshold for clippy to flag large error variants [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #17109:
URL: https://github.com/apache/datafusion/pull/17109#discussion_r2265209104
##########
datafusion/sqllogictest/src/engines/datafusion_engine/error.rs:
##########
@@ -28,7 +28,7 @@ pub type Result<T, E = DFSqlLogicTestError> = std::result::Result<T, E>;
pub enum DFSqlLogicTestError {
/// Error from sqllogictest-rs
#[error("SqlLogicTest error(from sqllogictest-rs crate): {0}")]
- SqlLogicTest(#[from] TestError),
+ SqlLogicTest(#[from] Box<TestError>),
Review Comment:
I agree it doesn't make a significant difference
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Set a lower threshold for clippy to flag large error variants [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #17109:
URL: https://github.com/apache/datafusion/pull/17109
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Set a lower threshold for clippy to flag large error variants [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17109:
URL: https://github.com/apache/datafusion/pull/17109#issuecomment-3174466539
Thanks again @Jefffrey and @xudong963
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org