Repository: ToucanToco/fastexcel (language: python)

Issue:
Silent deletion of data should be construed as a bug, not the default behavior
I just ran into the issue from #208 with someone I'm helping learn python. In fact that's actually the only reason we realised anything was wrong, because it was a small human readable practice dataset they knew some of the columns shouldn't have been completely empty.  There were no warnings, no error messages, just data going missing completely silently. Nothing in Polars' breaking changes documentation suggested this was a possibility, and nothing in Fastexcel's documentation warns of possible silent data loss either.

Imho inferring a "Null" column should be treated as a failure state. A user should need to explicitly opt-in via argument to allow the possible loss of data with a warning. Deleting an entire column of data  _completely silently_ because an excel spreadsheet just happened to have been sorted in a particular order is a showstopping issue. I can not state in strong enough terms how much I disagree with the choice to have this as a default behavior.



Discussion / hints:
Yeah I have to agree that the fact it is a default behaviour is problematic, should be opt in and at the bare minimum a warning should be posted.
@lukapeschke I don't think we should set to `None` by default the sample rows but we should probably raise a warning if a column as the "null" type. Or maybe try to load more data on those columns by default
As far as I know any conventional cell value in excel can be represented as a string. A safe option could be to default to string when a column type can't be inferred. That gives users a chance to explore the data with values presented kind of "as-is", rather than forcing them to open it by hand in excel.
Here the issue isn't that the type couldn't be inferred though.
The issue is that when null is wrongly inferred it end up in data loss.
I think a warning when null is inferred and/or having null inferred being opt in instead of default behaviour would be the best option.
We plan to add a fallback on string when dtype could not be inferred by default, which seems to be more reasonable indeed.
We'll do that in v0.13.0 since v0.12.0 is already planned for this month.
In the meantime you can set `schema_sample_rows=None` to ensure every cells have been taken into account
Hello, I referenced this issue in https://github.com/pola-rs/polars/issues/18612#issuecomment-2407485034, as the default schema inference fails when the number of empty records is high enough. 

You can use my example .xlsx file attached in the issue mentioned above. It has over 100k records for the id column but for the second column, only ~3k contain values and they appear first very far into the file, effectively rendering the column "null" under the current fastexcel behavior. This shouldn't work like it does.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 2afd562..5953dd4 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -55,6 +55,12 @@ dependencies = [
  "derive_arbitrary",
 ]
 
+[[package]]
+name = "arc-swap"
+version = "1.7.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "69f7f8c3906b62b754cd5326047894316021dcfe5a194c8ea52bdd94934a3457"
+
 [[package]]
 name = "arrow"
 version = "53.1.0"
@@ -412,8 +418,10 @@ dependencies = [
  "arrow",
  "calamine",
  "chrono",
+ "log",
  "pretty_assertions",
  "pyo3",
+ "pyo3-log",
  "rstest",
 ]
 
@@ -776,6 +784,17 @@ dependencies = [
  "pyo3-build-config",
 ]
 
+[[package]]
+name = "pyo3-log"
+version = "0.11.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3ac84e6eec1159bc2a575c9ae6723baa6ee9d45873e9bebad1e3ad7e8d28a443"
+dependencies = [
+ "arc-swap",
+ "log",
+ "pyo3",
+]
+
 [[package]]
 name = "pyo3-macros"
 version = "0.22.5"
diff --git a/Cargo.toml b/Cargo.toml
index 4a168aa..370e445 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -28,7 +28,9 @@ crate-type = ["cdylib"]
 arrow = { version = "53.1.0", default-features = false, features = ["pyarrow"] }
 calamine = { version = "0.26.1", features = ["dates"] }
 chrono = { version = "0.4.38", default-features = false }
+log = "0.4.22"
 pyo3 = { version = "0.22.5", features = ["abi3-py38"] }
+pyo3-log = "0.11.0"
 
 [dev-dependencies]
 pretty_assertions = "1.4.1"
diff --git a/pyproject.toml b/pyproject.toml
index 2af19c9..712ecd0 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -53,6 +53,8 @@ warn_unused_ignores = true
 
 [tool.pytest.ini_options]
 testpaths = ["python/tests"]
+log_cli = true
+log_cli_level = "INFO"
 
 [tool.ruff]
 line-length = 100
diff --git a/src/lib.rs b/src/lib.rs
index 299c1ee..74857f6 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -43,6 +43,8 @@ fn get_version() -> String {
 
 #[pymodule]
 fn _fastexcel(m: &Bound<'_, PyModule>) -> PyResult<()> {
+    pyo3_log::init();
+
     let py = m.py();
     m.add_function(wrap_pyfunction!(read_excel, m)?)?;
     m.add_class::<ColumnInfo>()?;
diff --git a/src/types/dtype.rs b/src/types/dtype.rs
index c7d630f..2027ee4 100644
--- a/src/types/dtype.rs
+++ b/src/types/dtype.rs
@@ -7,6 +7,7 @@ use std::{
 
 use arrow::datatypes::{DataType as ArrowDataType, TimeUnit};
 use calamine::{CellErrorType, CellType, DataType, Range};
+use log::warn;
 use pyo3::{
     prelude::PyAnyMethods, Bound, FromPyObject, PyAny, PyObject, PyResult, Python, ToPyObject,
 };
@@ -281,8 +282,13 @@ pub(crate) fn get_dtype_for_column<DT: CellType + Debug + DataType>(
     column_types.remove(&DType::Null);
 
     if column_types.is_empty() {
-        // If no type apart from NULL was found, it's a NULL column
-        Ok(DType::Null)
+        // If no type apart from NULL was found, fallback to string except if the column is empty
+        if start_row == end_row {
+            Ok(DType::Null)
+        } else {
+            warn!("Could not determine dtype for column {col}, falling back to string");
+            Ok(DType::String)
+        }
     } else if matches!(dtype_coercion, &DTypeCoercion::Strict) && column_types.len() != 1 {
         // If dtype coercion is strict and we do not have a single dtype, it's an error
         Err(


Test patch:
diff --git a/python/tests/fixtures/infer-dtypes-fallback.xlsx b/python/tests/fixtures/infer-dtypes-fallback.xlsx
new file mode 100644
index 0000000..70ba725
Binary files /dev/null and b/python/tests/fixtures/infer-dtypes-fallback.xlsx differ
diff --git a/python/tests/test_dtypes.py b/python/tests/test_dtypes.py
index 85c7c01..4767571 100644
--- a/python/tests/test_dtypes.py
+++ b/python/tests/test_dtypes.py
@@ -314,3 +314,42 @@ def test_one_dtype_for_all() -> None:
         ),
     ]
     assert sheet.to_polars().dtypes == [pl.String] * 7
+
+
+def test_fallback_infer_dtypes(mocker) -> None:
+    """it should fallback to string if it can't infer the dtype"""
+    import logging
+
+    logger_instance_mock = mocker.patch("logging.getLogger", autospec=True).return_value
+
+    excel_reader = fastexcel.read_excel(path_for_fixture("infer-dtypes-fallback.xlsx"))
+    sheet = excel_reader.load_sheet(0)
+
+    # Ensure a warning message was logged to explain the fallback to string
+    logger_instance_mock.makeRecord.assert_called_once_with(
+        "fastexcel.types.dtype",
+        logging.WARNING,
+        mocker.ANY,
+        mocker.ANY,
+        "Could not determine dtype for column 1, falling back to string",
+        mocker.ANY,
+        mocker.ANY,
+    )
+
+    assert sheet.available_columns == [
+        fastexcel.ColumnInfo(
+            name="id",
+            index=0,
+            dtype="float",
+            dtype_from="guessed",
+            column_name_from="looked_up",
+        ),
+        fastexcel.ColumnInfo(
+            name="label",
+            index=1,
+            dtype="string",
+            dtype_from="guessed",
+            column_name_from="looked_up",
+        ),
+    ]
+    assert sheet.to_polars().dtypes == [pl.Float64, pl.String]
diff --git a/test-requirements.txt b/test-requirements.txt
index 9b3a284..e743863 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -3,5 +3,6 @@ openpyxl>=3.1.2,<4
 pre-commit>=2.20.0,<5
 pytest>=7.1.3
 pytest-benchmark>=4.0.0,<5
+pytest-mock>=3.1
 ruff>=0.7,<0.8
 xlrd>=2.0.1,<3