Skip to content

New lint clippy::allow_attributes #18881

Closed

Description

@Jefffrey

Is your feature request related to a problem or challenge?

Lint reference: https://rust-lang.github.io/rust-clippy/master/index.html?search=allow_att#allow_attributes

Checks for usage of the #[allow] attribute and suggests replacing it with the #[expect] attribute (See RFC 2383)

#[expect] attributes suppress the lint emission, but emit a warning, if the expectation is unfulfilled. This can be useful to be notified when the lint is no longer triggered.

We have a quite a few occurrences of #[allow] in our codebase, but should prefer #[expect] instead for the reasons above.

Describe the solution you'd like

Similar to #18503 we should roll this lint our gradually through crates before finally enabling at the workspace level so we don't have one big PR.

Describe alternatives you've considered

Don't enable this lint if we don't see value or if the false positive rate is too high (see below).

Additional context

I tested it for some of our current #[allow]s and it has some false positives, especially for dead_code and deprecated.

Example 1

/// create an in-memory table given the partition len, array len, and batch size,
/// and the result table will be of array_len in total, and then partitioned, and batched.
#[allow(dead_code)]
pub fn create_table_provider(

  • If I switch this to #[expect(dead_code)] then clippy will flag it as this lint expectation is unfulfilled. But if I remove the lint then the dead code warning pops up anyway 🤔

Example 2

/// Configuration for temporary disk access
#[allow(deprecated)]
#[deprecated(since = "48.0.0", note = "Use DiskManagerBuilder instead")]
#[derive(Debug, Clone, Default)]
pub enum DiskManagerConfig {
/// Use the provided [DiskManager] instance
Existing(Arc<DiskManager>),
/// Create a new [DiskManager] that creates temporary files within
/// a temporary directory chosen by the OS
#[default]
NewOs,
/// Create a new [DiskManager] that creates temporary files within
/// the specified directories
NewSpecified(Vec<PathBuf>),
/// Disable disk manager, attempts to create temporary files will error
Disabled,
}

  • Switching this to #[expect(deprecated)] causes similar to above, but also flags NewOS variant as using deprecated variant. Something to do with how Default is derived?

(I say false positive but it seems more like a bug in clippy 🤔)

Maybe there are solutions for the above, I haven't looked into it much yet.

Another thing to keep in mind is how features interact, for example here:

fn get_metadata<'a>(
&'a mut self,
#[allow(unused_variables)] options: Option<&'a ArrowReaderOptions>,
) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {
let object_meta = self.partitioned_file.object_meta.clone();
let metadata_cache = Arc::clone(&self.metadata_cache);
async move {
#[cfg(feature = "parquet_encryption")]
let file_decryption_properties = options
.and_then(|o| o.file_decryption_properties())
.map(Arc::clone);
#[cfg(not(feature = "parquet_encryption"))]
let file_decryption_properties = None;

  • This #[allow] is for when parquet_encryption feature is not enabled, but if we change it to #[expect] blindly then I believe it'll cause issues when parquet_encryption is enabled, so we need to be careful

Tasks

Note: we would welcome PRs that can do multiple crates in a single PR (so long as the diff isn't too large). Felt a bit overwhelmed by the volume and speed of PRs that came in for #18503 😅

Activity

  1. added theissue type on Nov 22, 2025
  2. Jefffrey commented on Nov 22, 2025

    @Jefffrey
    ContributorAuthor

    Thoughts @2010YOUY01 ?

  3. 2010YOUY01 commented on Nov 23, 2025

    @2010YOUY01
    Contributor

    If there is a good way to deal with the mentioned feature flag issue, I think we should add this lint rule.

  4. Jefffrey commented on Nov 23, 2025

    @Jefffrey
    ContributorAuthor

    If there is a good way to deal with the mentioned feature flag issue, I think we should add this lint rule.

    It would look something like this:

        fn get_metadata<'a>(
            &'a mut self,
            #[cfg_attr(not(feature = "parquet_encryption"), expect(unused_variables))]
            options: Option<&'a ArrowReaderOptions>,
        ) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {

    Doable, but a little verbose.

  5. 2010YOUY01 commented on Nov 24, 2025

    @2010YOUY01
    Contributor

    If there is a good way to deal with the mentioned feature flag issue, I think we should add this lint rule.

    It would look something like this:

    fn get_metadata<'a>(
        &'a mut self,
        #[cfg_attr(not(feature = "parquet_encryption"), expect(unused_variables))]
        options: Option<&'a ArrowReaderOptions>,
    ) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> {
    

    Doable, but a little verbose.

    I think this marker describes the arg quite precisely: 'this arg is only used in parquet_encryption' feature.

    So I'm +1 for this lint. Maybe we should add comment like the above explanation, to deal with the verbosity.

  6. Jefffrey commented on Nov 24, 2025

    @Jefffrey
    ContributorAuthor

    I've marked it as good first issue, but note to anyone interested, please do not assign this issue to yourself as it is meant to be a tracking issue. Just pick a crate (or crates) that is not yet done/in-progress and leave a comment with your intention to work on those crates (to avoid overlap).

  7. YuraLitvinov commented on Nov 24, 2025

    @YuraLitvinov
    Contributor

    Hello, if it's okay I am taking the physical-plan.

  8. carlosahs commented on Nov 27, 2025

    @carlosahs
    Contributor

    Hi! If no one else is working on datafusion-functions-nested, I can take that.

  9. chakkk309 commented on Nov 27, 2025

    @chakkk309
    Contributor

    Hi all, I would like to take the following crates:

    • datafusion-catalog
    • datafusion-listing
    • datafusion-common
    • datafusion-common-runtime
    • datafusion-datasource
    • datafusion-datasource-arrow
    • datafusion-datasource-avro
    • datafusion-datasource-csv
    • datafusion-datasource-json
    • datafusion-datasource-parquet
    • datafusion-doc
    • datafusion-execution
    • datafusion-expr
    • datafusion-expr-common
    • datafusion-ffi
  10. carlosahs commented on Nov 27, 2025

    @carlosahs
    Contributor

    @Jefffrey, for datafusion-functions-nested it suffices to add #![deny(clippy::allow_attributes)] as there is no usage of #[allow]. The same applies to datafusion-functions-table. If it's ok, I'll create a PR to enforce clippy::allow_attributes for all datafusion-functions-* crates as it looks like usage of #[allow] is low and to avoid raising multiple 1-liner PRs.

  11. Jefffrey commented on Nov 28, 2025

    @Jefffrey
    ContributorAuthor

    @Jefffrey, for datafusion-functions-nested it suffices to add #![deny(clippy::allow_attributes)] as there is no usage of #[allow]. The same applies to datafusion-functions-table. If it's ok, I'll create a PR to enforce clippy::allow_attributes for all datafusion-functions-* crates as it looks like usage of #[allow] is low and to avoid raising multiple 1-liner PRs.

    That sounds good, thanks

  12. 14 remaining items

  13. kumarUjjawal commented on Dec 13, 2025

    @kumarUjjawal
    Contributor

    I'm tackling:

    • datafusion-spark

    • datafusion-sql

    • datafusion-substrait

  14. kumarUjjawal commented on Dec 13, 2025

    @kumarUjjawal
    Contributor

    @Jefffrey I opened a PR with changes to three of the crates, do take a look when you get the time.

  15. kumarUjjawal commented on Dec 13, 2025

    @kumarUjjawal
    Contributor

    @Jefffrey Opened PR for few other crates:

    • datafusion-ffi
    • datafusion-macros
    • datafusion-optimizer
  16. kumarUjjawal commented on Dec 13, 2025

    @kumarUjjawal
    Contributor

    @Jefffrey I looked into the reamining three and I think they don't require the changes.

    • datafusion-proto: We should keep the parquet max_statistics_size lint as #[allow(deprecated)] because the deprecation isn’t emitted in all builds, and using expect causes unfulfilled-lint failures; thre are no other changes needed.

    • datafusion-pruning: The two #[allow(dead_code)] in pruning predicates because they cover parquet-only code paths and would risk unfulfilled expectations when the feature is disabled.

    • datafusion-session: no allow/expect present.

  17. Jefffrey commented on Dec 14, 2025

    @Jefffrey
    ContributorAuthor

    @Jefffrey I looked into the reamining three and I think they don't require the changes.

    * `datafusion-proto`: We should keep the parquet `max_statistics_size` lint as `#[allow(deprecated)]` because the deprecation isn’t emitted in all builds, and using expect causes unfulfilled-lint failures; thre are no other changes needed.
    
    * `datafusion-pruning`: The two `#[allow(dead_code)]` in pruning predicates because they cover parquet-only code paths and would risk unfulfilled expectations when the feature is disabled.
    
    * `datafusion-session`: no allow/expect present.
    

    We should still apply the deny at their lib.rs level, and any exceptions we handle on a case by case basis, so I still expect PRs for these crates.

    But it sounds like these can be fixed if we ensure the lint is expected only when certain features are enabled, see: #18881 (comment)

  18. carlosahs commented on Dec 14, 2025

    @carlosahs
    Contributor

    @kumarUjjawal, let's remember that the purpose of expect is to be identical to allow, but will cause a lint to be emitted when the code it decorates does not raise a lint warning. With this in mind I want to highlight the following:

    • datafusion-proto: We should remove the #[allow(deprecated)] lints as max_statistics_size no longer exists (ref). expect is essentially helping us to identify that we don't need to add the deprecated attribute because there are no more deprecated fields for ParquetOptions:
      /// Options for reading and writing parquet files
      ///
      /// See also: [`SessionConfig`]
      ///
      /// [`SessionConfig`]: https://docs.rs/datafusion/latest/datafusion/prelude/struct.SessionConfig.html
      pub struct ParquetOptions {
      // The following options affect reading parquet files
      /// (reading) If true, reads the Parquet data page level metadata (the
      /// Page Index), if present, to reduce the I/O and number of
      /// rows decoded.
      pub enable_page_index: bool, default = true
      /// (reading) If true, the parquet reader attempts to skip entire row groups based
      /// on the predicate in the query and the metadata (min/max values) stored in
      /// the parquet file
      pub pruning: bool, default = true
      /// (reading) If true, the parquet reader skip the optional embedded metadata that may be in
      /// the file Schema. This setting can help avoid schema conflicts when querying
      /// multiple parquet files with schemas containing compatible types but different metadata
      pub skip_metadata: bool, default = true
      /// (reading) If specified, the parquet reader will try and fetch the last `size_hint`
      /// bytes of the parquet file optimistically. If not specified, two reads are required:
      /// One read to fetch the 8-byte parquet footer and
      /// another to fetch the metadata length encoded in the footer
      /// Default setting to 512 KiB, which should be sufficient for most parquet files,
      /// it can reduce one I/O operation per parquet file. If the metadata is larger than
      /// the hint, two reads will still be performed.
      pub metadata_size_hint: Option<usize>, default = Some(512 * 1024)
      /// (reading) If true, filter expressions are be applied during the parquet decoding operation to
      /// reduce the number of rows decoded. This optimization is sometimes called "late materialization".
      pub pushdown_filters: bool, default = false
      /// (reading) If true, filter expressions evaluated during the parquet decoding operation
      /// will be reordered heuristically to minimize the cost of evaluation. If false,
      /// the filters are applied in the same order as written in the query
      pub reorder_filters: bool, default = false
      /// (reading) Force the use of RowSelections for filter results, when
      /// pushdown_filters is enabled. If false, the reader will automatically
      /// choose between a RowSelection and a Bitmap based on the number and
      /// pattern of selected rows.
      pub force_filter_selections: bool, default = false
      /// (reading) If true, parquet reader will read columns of `Utf8/Utf8Large` with `Utf8View`,
      /// and `Binary/BinaryLarge` with `BinaryView`.
      pub schema_force_view_types: bool, default = true
      /// (reading) If true, parquet reader will read columns of
      /// `Binary/LargeBinary` with `Utf8`, and `BinaryView` with `Utf8View`.
      ///
      /// Parquet files generated by some legacy writers do not correctly set
      /// the UTF8 flag for strings, causing string columns to be loaded as
      /// BLOB instead.
      pub binary_as_string: bool, default = false
      /// (reading) If true, parquet reader will read columns of
      /// physical type int96 as originating from a different resolution
      /// than nanosecond. This is useful for reading data from systems like Spark
      /// which stores microsecond resolution timestamps in an int96 allowing it
      /// to write values with a larger date range than 64-bit timestamps with
      /// nanosecond resolution.
      pub coerce_int96: Option<String>, transform = str::to_lowercase, default = None
      /// (reading) Use any available bloom filters when reading parquet files
      pub bloom_filter_on_read: bool, default = true
      /// (reading) The maximum predicate cache size, in bytes. When
      /// `pushdown_filters` is enabled, sets the maximum memory used to cache
      /// the results of predicate evaluation between filter evaluation and
      /// output generation. Decreasing this value will reduce memory usage,
      /// but may increase IO and CPU usage. None means use the default
      /// parquet reader setting. 0 means no caching.
      pub max_predicate_cache_size: Option<usize>, default = None
      // The following options affect writing to parquet files
      // and map to parquet::file::properties::WriterProperties
      /// (writing) Sets best effort maximum size of data page in bytes
      pub data_pagesize_limit: usize, default = 1024 * 1024
      /// (writing) Sets write_batch_size in bytes
      pub write_batch_size: usize, default = 1024
      /// (writing) Sets parquet writer version
      /// valid values are "1.0" and "2.0"
      pub writer_version: String, default = "1.0".to_string()
      /// (writing) Skip encoding the embedded arrow metadata in the KV_meta
      ///
      /// This is analogous to the `ArrowWriterOptions::with_skip_arrow_metadata`.
      /// Refer to <https://docs.rs/parquet/53.3.0/parquet/arrow/arrow_writer/struct.ArrowWriterOptions.html#method.with_skip_arrow_metadata>
      pub skip_arrow_metadata: bool, default = false
      /// (writing) Sets default parquet compression codec.
      /// Valid values are: uncompressed, snappy, gzip(level),
      /// lzo, brotli(level), lz4, zstd(level), and lz4_raw.
      /// These values are not case sensitive. If NULL, uses
      /// default parquet writer setting
      ///
      /// Note that this default setting is not the same as
      /// the default parquet writer setting.
      pub compression: Option<String>, transform = str::to_lowercase, default = Some("zstd(3)".into())
      /// (writing) Sets if dictionary encoding is enabled. If NULL, uses
      /// default parquet writer setting
      pub dictionary_enabled: Option<bool>, default = Some(true)
      /// (writing) Sets best effort maximum dictionary page size, in bytes
      pub dictionary_page_size_limit: usize, default = 1024 * 1024
      /// (writing) Sets if statistics are enabled for any column
      /// Valid values are: "none", "chunk", and "page"
      /// These values are not case sensitive. If NULL, uses
      /// default parquet writer setting
      pub statistics_enabled: Option<String>, transform = str::to_lowercase, default = Some("page".into())
      /// (writing) Target maximum number of rows in each row group (defaults to 1M
      /// rows). Writing larger row groups requires more memory to write, but
      /// can get better compression and be faster to read.
      pub max_row_group_size: usize, default = 1024 * 1024
      /// (writing) Sets "created by" property
      pub created_by: String, default = concat!("datafusion version ", env!("CARGO_PKG_VERSION")).into()
      /// (writing) Sets column index truncate length
      pub column_index_truncate_length: Option<usize>, default = Some(64)
      /// (writing) Sets statistics truncate length. If NULL, uses
      /// default parquet writer setting
      pub statistics_truncate_length: Option<usize>, default = Some(64)
      /// (writing) Sets best effort maximum number of rows in data page
      pub data_page_row_count_limit: usize, default = 20_000
      /// (writing) Sets default encoding for any column.
      /// Valid values are: plain, plain_dictionary, rle,
      /// bit_packed, delta_binary_packed, delta_length_byte_array,
      /// delta_byte_array, rle_dictionary, and byte_stream_split.
      /// These values are not case sensitive. If NULL, uses
      /// default parquet writer setting
      pub encoding: Option<String>, transform = str::to_lowercase, default = None
      /// (writing) Write bloom filters for all columns when creating parquet files
      pub bloom_filter_on_write: bool, default = false
      /// (writing) Sets bloom filter false positive probability. If NULL, uses
      /// default parquet writer setting
      pub bloom_filter_fpp: Option<f64>, default = None
      /// (writing) Sets bloom filter number of distinct values. If NULL, uses
      /// default parquet writer setting
      pub bloom_filter_ndv: Option<u64>, default = None
      /// (writing) Controls whether DataFusion will attempt to speed up writing
      /// parquet files by serializing them in parallel. Each column
      /// in each row group in each output file are serialized in parallel
      /// leveraging a maximum possible core count of n_files*n_row_groups*n_columns.
      pub allow_single_file_parallelism: bool, default = true
      /// (writing) By default parallel parquet writer is tuned for minimum
      /// memory usage in a streaming execution plan. You may see
      /// a performance benefit when writing large parquet files
      /// by increasing maximum_parallel_row_group_writers and
      /// maximum_buffered_record_batches_per_stream if your system
      /// has idle cores and can tolerate additional memory usage.
      /// Boosting these values is likely worthwhile when
      /// writing out already in-memory data, such as from a cached
      /// data frame.
      pub maximum_parallel_row_group_writers: usize, default = 1
      /// (writing) By default parallel parquet writer is tuned for minimum
      /// memory usage in a streaming execution plan. You may see
      /// a performance benefit when writing large parquet files
      /// by increasing maximum_parallel_row_group_writers and
      /// maximum_buffered_record_batches_per_stream if your system
      /// has idle cores and can tolerate additional memory usage.
      /// Boosting these values is likely worthwhile when
      /// writing out already in-memory data, such as from a cached
      /// data frame.
      pub maximum_buffered_record_batches_per_stream: usize, default = 2
      }
      . Timeline of max_statistics_size:
      1. Deprecate max statistics size properly #14188.
      2. Upgrade arrow/parquet to 56.0.0 #16690.
    • datafusion-pruning: For this one I think we need to first figure out if this is only used by parquet feature right now claims are still valid. pruning_predicate.rs was created from logic initially living in lib.rs as part of this refactor: refactor: move PruningPredicate into its own module #16587. And that PR was from Jun 27. It's likely claim is no longer valid since expect is failing with unfulfilled lint expectation errors, in which case I think we should remove the #![allow(dead_code)]
    • datafusion-session: I think it's best to enable #![deny(clippy::allow_attributes)] for future use cases. Case from datafusion-proto suggests if expect was used instead of allow, then deprecated attributes would've been removed too when max_statistics_size was removed.

    My suggestion when facing -D unfulfilled-lint-expectations errors is to first identify possible root causes, then track down the root cause, and finally document why an allow needs to be removed based on such findings.

  19. kumarUjjawal commented on Dec 16, 2025

    @kumarUjjawal
    Contributor

    Opened a PR for:

    • datafusion-proto
    • datafusion-pruning
    • datafusion-session
  20. self-assigned this
    on Dec 31, 2025
  21. added a commit that references this issue on Jan 28, 2026
    5f9bacd
  22. added a commit that references this issue on Mar 24, 2026
    413d963
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions