[DISCUSSION] Should DataFusion project use `lib.rs` pattern/antipattern?

[comphead]

Is your feature request related to a problem or challenge?

There is an article I found pretty recently https://kerkour.com/rust-patterns-dont-put-code-in-lib-mod-files

Which suggests using certain file names instead package/lib.rs and I'm feeling it has some reasoning behind it. Even in Github when you search by file and this is often a lib.rs you got a pile of lib.rs's

Just wondering what you guys thinking about it.

@alamb @andygrove @viirya @mustafasrepo @ozankabak @jayzhan211 @waynexia

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

[tisonkun]

It would be good not to hard require it but split down lib.rs/mod.rs when it grows.

[alamb]

There is an article I found pretty recently https://kerkour.com/rust-patterns-dont-put-code-in-lib-mod-files

I think the pattern suggested in this article sounds great to me.

The major concern I have is trying to change people's behavior with processes, etc.

I think improving the code modularity and then leaving comments to encourage people to continue to follow the pattern would likely work well

[jayzhan211]

Because it's far easier to find code with the Search file feature of your code editor when it's in a descriptive filename

Given the reason in the article, instead of naming it mod.rs under the crate like crate/mod.rs, we should have a file first and then create subcrate based on it. Therefore, we have crate.rs and crate/subcrate.rs instead of crate/mod.rs and crate/subcrate.rs

I found a great image of describing module hierarchy

https://www.reddit.com/r/rust/comments/ujry0b/media_how_to_create_a_module_hierarchy_in_rust/

In this way, it is possible that no mod.rs exist anymore. And, lib.rs just contains bunch of mod and use statement.

I add the quick example here https://github.com/apache/datafusion/pull/11376/files

[waynexia]

Compared with putting lots of logic in lib.rs, splitting it into several dedicated modules does make it easier to read and maintain. This kind of refactor keeps happening in our codebase which is very 🚀 and I agree we should keep doing this.

The name of sub-module's root file (whether mod.rs or some_func.rs) is another pattern discussion. GreptimeDB uses some_func.rs instead of mod.rs whereas DataFusion has more mod.rs. This is closer to style taste and IMO has less effect on maintainability or readability.

[comphead]

Now DF has 22 lib.rs, 90 mod.rs, and the mod.rs is more overused then lib.rs. Speaking in lines of code

git ls-files | grep lib.rs | xargs wc -l | sort -n -s -k 1,1nr
  3403 total
    1017 datafusion/physical-plan/src/lib.rs
     645 datafusion/core/src/lib.rs
     211 datafusion/functions-aggregate/src/lib.rs
     211 datafusion/functions/src/lib.rs
     183 datafusion/functions-array/src/lib.rs
     131 datafusion/proto/src/lib.rs
     128 datafusion/wasmtest/src/lib.rs
     125 test-utils/src/lib.rs
      99 datafusion/expr/src/lib.rs
      98 datafusion/common/src/lib.rs
      93 datafusion/proto-common/src/lib.rs
      81 datafusion/substrait/src/lib.rs
      75 datafusion/optimizer/src/lib.rs
      73 datafusion/physical-expr/src/lib.rs
      46 datafusion/sql/src/lib.rs
      40 datafusion/execution/src/lib.rs
      31 datafusion-cli/src/lib.rs
      28 datafusion/sqllogictest/src/lib.rs
      25 datafusion/physical-expr-common/src/lib.rs
      24 benchmarks/src/lib.rs
      20 datafusion/common-runtime/src/lib.rs
      19 docs/src/lib.rs
git ls-files | grep mod.rs | xargs wc -l | sort -n -s -k 1,1nr
   47119 total
    6549 datafusion/common/src/scalar/mod.rs
    3549 datafusion/core/src/dataframe/mod.rs
    2377 datafusion/physical-plan/src/aggregates/mod.rs
    2358 datafusion/core/tests/dataframe/mod.rs
    2122 datafusion/core/src/datasource/physical_plan/parquet/mod.rs
    2056 datafusion/proto/src/physical_plan/mod.rs
    1996 datafusion/core/src/execution/context/mod.rs
    1966 datafusion/optimizer/src/optimize_projections/mod.rs
    1675 datafusion/proto/src/logical_plan/mod.rs
    1670 datafusion/physical-plan/src/repartition/mod.rs
    1431 datafusion/core/tests/parquet/mod.rs
    1166 datafusion/physical-plan/src/windows/mod.rs
    1127 datafusion/proto-common/src/from_proto/mod.rs
    1069 datafusion/sql/src/expr/mod.rs
    1036 datafusion/proto-common/src/to_proto/mod.rs
    1030 datafusion/common/src/utils/mod.rs
     778 datafusion/core/src/datasource/physical_plan/mod.rs
     756 datafusion/core/tests/memory_limit/mod.rs
     683 datafusion/physical-plan/src/metrics/mod.rs
     644 datafusion/physical-plan/src/topk/mod.rs
     552 datafusion/physical-expr-common/src/aggregate/mod.rs
     551 datafusion/physical-expr/src/utils/mod.rs
     543 datafusion/physical-expr/src/equivalence/mod.rs
     494 datafusion/expr/src/expr_rewriter/mod.rs
     442 datafusion/core/src/catalog/mod.rs
     438 datafusion/core/src/test/mod.rs
     414 datafusion/core/tests/fifo/mod.rs
     407 datafusion/execution/src/memory_pool/mod.rs
     403 datafusion/core/tests/expr_api/mod.rs
     379 datafusion/core/src/datasource/file_format/mod.rs
     368 datafusion/core/src/test_util/mod.rs
     353 datafusion/sqllogictest/src/engines/postgres_engine/mod.rs
     343 datafusion/core/tests/sql/mod.rs
     317 datafusion/common/src/file_options/mod.rs
     313 datafusion/proto/src/bytes/mod.rs
     312 datafusion/core/tests/custom_sources_cases/mod.rs
     287 datafusion/functions/src/datetime/mod.rs
     273 datafusion/optimizer/src/test/mod.rs
     266 datafusion/core/src/datasource/listing/mod.rs
     263 datafusion/sql/tests/common/mod.rs
     243 datafusion/core/benches/data_utils/mod.rs
     222 datafusion/physical-expr/src/expressions/mod.rs
     196 datafusion/functions/src/string/mod.rs
     190 benchmarks/src/tpch/mod.rs
     180 datafusion/optimizer/src/analyzer/mod.rs
     176 datafusion/functions/src/math/mod.rs
     146 datafusion/sql/src/relation/mod.rs
     143 datafusion/functions/src/unicode/mod.rs
     131 datafusion/common/src/display/mod.rs
     128 datafusion/physical-plan/src/aggregates/order/mod.rs
     100 datafusion/functions/src/core/mod.rs
      92 datafusion/core/src/datasource/mod.rs
      88 datafusion/core/src/datasource/file_format/write/mod.rs
      87 datafusion/expr/src/type_coercion/mod.rs
      85 datafusion/physical-plan/src/aggregates/group_values/mod.rs
      71 datafusion/functions/src/regex/mod.rs
      67 datafusion/functions/src/crypto/mod.rs
      56 datafusion/physical-plan/src/joins/mod.rs
      52 datafusion/execution/src/cache/mod.rs
      51 datafusion/expr/src/logical_plan/mod.rs
      49 datafusion/core/src/datasource/avro_to_arrow/mod.rs
      46 datafusion/sql/src/unparser/mod.rs
      45 datafusion/core/src/physical_optimizer/mod.rs
      42 datafusion/functions/src/encoding/mod.rs
      39 datafusion/core/tests/macro_hygiene/mod.rs
      39 datafusion/physical-expr/src/window/mod.rs
      35 datafusion/optimizer/src/simplify_expressions/mod.rs
      35 datafusion/physical-expr/src/aggregate/mod.rs
      34 datafusion/core/tests/user_defined/mod.rs
      31 datafusion/physical-expr/src/aggregate/groups_accumulator/mod.rs
      31 datafusion/physical-plan/src/sorts/mod.rs
      29 datafusion/sqllogictest/src/engines/mod.rs
      28 datafusion/proto/src/generated/mod.rs
      27 datafusion/core/src/execution/mod.rs
      26 datafusion/core/tests/fuzz_cases/mod.rs
      25 benchmarks/src/util/mod.rs
      25 datafusion/proto-common/src/generated/mod.rs
      24 datafusion/sqllogictest/src/engines/datafusion_engine/mod.rs
      23 datafusion/physical-expr-common/src/aggregate/count_distinct/mod.rs
      23 datafusion/physical-expr-common/src/expressions/mod.rs
      23 datafusion/substrait/tests/cases/mod.rs
      22 datafusion/physical-expr-common/src/aggregate/groups_accumulator/mod.rs
      22 datafusion/physical-expr/src/intervals/mod.rs
      22 datafusion/physical-plan/src/aggregates/topk/mod.rs
      20 datafusion/core/src/variable/mod.rs
      20 datafusion/proto/tests/cases/mod.rs
      19 datafusion/substrait/src/logical_plan/mod.rs
      19 datafusion/substrait/src/physical_plan/mod.rs
      18 datafusion/expr/src/test/mod.rs
      18 datafusion/sql/tests/cases/mod.rs

I agree with @jayzhan211 and @waynexia we should probably start decomposing mod.rs/lib.rs into smaller files with more meaningful names/paths/crates.

Speaking to the mod files, it is often huge size because of tests enclosed. Rust gives the comfy system of having tests in the same file where the main code written, so you dont to navigate back and forth, but for big files I'm not sure what approach is the better.

[alamb]

I likewise like smaller modules with more focused code. I also find huge modules hard to navigate

[shubhamdagar9854]

Example - Before (Traditional Approach)
src/
│── lib.rs <-- Main entry point, contains logic
│── models.rs
│── utils.rs

Example - After (Alternative Approach)
src/
│── backend.rs <-- More descriptive than "lib.rs"
│── models.rs
│── utils.rs
│── lib.rs <-- Only re-exports modules

// lib.rs (Minimal)
pub mod backend;

This approach makes sense for large projects, but in small projects, lib.rs is still a simple and effective way to structure code. It depends on the scale and organization needs of the project.