Improve Excek reading... again

Author: Quafadas

scautable/src-jvm/ExcelMacros.scala

@@ -10,7 +10,8 @@ import java.io.File
 import io.github.quafadas.scautable.BadTableException
 import io.github.quafadas.scautable.InferrerOps
 
-/** Compile-time macro functions for reading Excel files These macros perform Excel file inspection at compile time to determine structure
+/** Compile-time macro functions for reading     val initial = ColumnTypeInfo()
+    val finalInfo = cells.foldLeft(initial)(updateTypeInfo) files These macros perform Excel file inspection at compile time to determine structure
   */
 object ExcelMacros:
 
@@ -111,8 +112,8 @@ object ExcelMacros:
               inferredTypeRepr.asType match
                 case '[v] => constructWithTypes[hdrs & Tuple, v & Tuple]
               end match
-            case _ =>
-              report.throwError("TypeInferrer not found")
+            case other =>
+              report.throwError(s"TypeInferrer not found: ${other}")
         case _ =>
           report.throwError(s"Could not summon Type for type: ${tupleExpr2.show}")
       end match
@@ -202,26 +203,30 @@ object ExcelMacros:
     val workbook = WorkbookFactory.create(new File(filePath))
     try
       val sheet = workbook.getSheet(sheetName)
-      val sheetIterator = sheet.iterator().asScala
-
-      // Skip header row
-      if sheetIterator.hasNext then sheetIterator.next()
-      end if
-
-      val sampleRows = sheetIterator.take(numRows).toList
 
       // Extract data based on column range or use all columns
       val columnData: List[List[Cell]] = colRange match
         case Some(range) if range.nonEmpty =>
           val cellRange = CellRangeAddress.valueOf(range)
+          val firstRow = cellRange.getFirstRow
+          val lastRow = cellRange.getLastRow
           val firstCol = cellRange.getFirstColumn
           val lastCol = cellRange.getLastColumn
-          sampleRows.map { row =>
+          
+          // Read only the specific rows from the range (including headers)
+          val targetRows = (firstRow to lastRow).map(sheet.getRow).filter(_ != null).toList
+          
+          targetRows.map { row =>
             (firstCol to lastCol).map { i =>
               row.getCell(i, Row.MissingCellPolicy.CREATE_NULL_AS_BLANK)
             }.toList
           }
         case _ =>
+          val sheetIterator = sheet.iterator().asScala
+          // Skip header row
+          if sheetIterator.hasNext then sheetIterator.next()
+          end if
+          val sampleRows = sheetIterator.take(numRows).toList
           sampleRows.map { row =>
             // Ensure we extract exactly headers.length columns to match headers
             (0 until headers.length).map { i =>
@@ -233,8 +238,11 @@ object ExcelMacros:
       val columns = columnData.transpose
 
       // Infer type for each column using Apache POI cell types
+      // SKIP THE FIRST ROW (header row) for type inference
       val columnTypes: List[TypeRepr] = columns.map { columnCells =>
-        inferColumnTypeFromCells(columnCells, preferIntToBoolean)
+        // TODO Interaction with HeaderOptions
+        val dataRows = columnCells.drop(1) // Skip header row
+        inferColumnTypeFromCells(dataRows, preferIntToBoolean)
       }
 
       // Build tuple type from column types
@@ -243,8 +251,14 @@ object ExcelMacros:
       }
 
       tupleType
-    finally workbook.close()
-    end try
+    finally 
+      try 
+        workbook.close()
+      catch
+        case _: Exception => 
+          // Workbook close can fail for Excel files with corrupted drawings - this is expected
+          println(s"Warning: Could not close workbook for file: $filePath")
+      end try
   end inferTypesFromExcelDataDirect
 
   /** Infer the most appropriate Scala type for a column based on Apache POI cell types
@@ -286,10 +300,11 @@ object ExcelMacros:
             )
           else
             val numericValue = cell.getNumericCellValue
-            val isWholeNumber = numericValue == numericValue.toLong
+            val isWholeNumber = numericValue == numericValue.toLong && !numericValue.isInfinite && !numericValue.isNaN
             info.copy(
               couldBeInt = info.couldBeInt && isWholeNumber && numericValue >= Int.MinValue && numericValue <= Int.MaxValue,
               couldBeLong = info.couldBeLong && isWholeNumber && numericValue >= Long.MinValue && numericValue <= Long.MaxValue,
+              couldBeDouble = info.couldBeDouble && !numericValue.isInfinite && !numericValue.isNaN, // All finite numeric values can be Double
               // Be more conservative with Boolean inference for numeric cells - only if it's exactly 0 or 1
               couldBeBoolean = info.couldBeBoolean && isWholeNumber && (numericValue == 0.0 || numericValue == 1.0)
             )
@@ -331,9 +346,18 @@ object ExcelMacros:
     val initial = ColumnTypeInfo()
     val finalInfo = cells.foldLeft(initial)(updateTypeInfo)
 
+    // // Debug output to understand type inference
+    // println(s"DEBUG inferColumnTypeFromCells: ${cells.length} cells")
+    // cells.take(3).foreach { cell =>
+    //   println(s"  Cell type: ${cell.getCellType}, value: '${cell.toString}'")
+    // }
+    // println(s"  Final: couldBeInt=${finalInfo.couldBeInt}, couldBeDouble=${finalInfo.couldBeDouble}, couldBeBoolean=${finalInfo.couldBeBoolean}, seenEmpty=${finalInfo.seenEmpty}")
+
     // Determine the most appropriate type based on what the column could be
-    val baseType =
-      if preferIntToBoolean then
+    val baseType = 
+      // If we have no cells, default to String (safest option)
+      if cells.isEmpty then TypeRepr.of[String]
+      else if preferIntToBoolean then
         if finalInfo.couldBeInt then TypeRepr.of[Int]
         else if finalInfo.couldBeBoolean then TypeRepr.of[Boolean]
         else if finalInfo.couldBeLong then TypeRepr.of[Long]
@@ -352,5 +376,4 @@ object ExcelMacros:
     if finalInfo.seenEmpty then TypeRepr.of[Option].appliedTo(baseType) else baseType
     end if
   end inferColumnTypeFromCells
-
 end ExcelMacros

scautable/test/resources/Numbers.xlsx

@@ -4,6 +4,15 @@ package io.github.quafadas.scautable
 import scala.NamedTuple.*
 
 import io.github.quafadas.table.{*, given}
+
+
+/**
+ * The cell contents of "Numbers.xlsx"
+ * 
+Doubles	Int	Longs	Strings
+1.10	1.00	1.00	blah
+2.20	2.00	3.00	blah  
+ */
 class ExcelSuite extends munit.FunSuite:
 
   test("excel provider compiles and typechecks") {
@@ -26,7 +35,7 @@ class ExcelSuite extends munit.FunSuite:
   }
 
   test("Numbers") {
-    val csv2 = Excel.resource("Numbers.xlsx", "Sheet1", TypeInferrer.FromAllRows)
+    val csv2 = Excel.resource("Numbers.xlsx", "Sheet1", "A1:D3", TypeInferrer.FromAllRows)
     csv2.toSeq.ptbln
     val csv = Excel.resource("Numbers.xlsx", "Sheet1", "A1:C3", TypeInferrer.FromAllRows)
     val seq = csv.toSeq
@@ -121,33 +130,33 @@ class ExcelSuite extends munit.FunSuite:
 
   test("excel provider with TypeInferrer.FirstRow should infer types from first row") {
     // Test FirstRow with Numbers.xlsx - should be equivalent to FirstN(1)
-    def csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FirstRow)
+    val csv = Excel.resource("Numbers.xlsx", "Sheet1", "A1:D3", TypeInferrer.FirstRow)
 
     // Verify that we can read the data and it compiles with inferred types
-    val rows = csv.toList
+    val rows: List[(Doubles : Double, Int : Int, Longs : Int, Strings : String)] = csv.toList
     assertEquals(rows.size, 2)
 
     // Test access to columns with the inferred types (should be same as FirstN(1))
-    assertEquals(csv.column["Doubles"].toList.head, 1.1) // Double
-    assertEquals(csv.column["Strings"].toList.head, "blah") // String
-    assertEquals(csv.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
-    assertEquals(csv.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Doubles"].toList.head, 1.1) // Double
+    assertEquals(rows.column["Strings"].toList.head, "blah") // String
+    assertEquals(rows.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
   }
 
   test("excel provider with TypeInferrer.FromAllRows should infer types from all rows") {
     // Test FromAllRows with Numbers.xlsx - should consider all data rows for type inference
-    def csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FromAllRows)
+    val csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FromAllRows)
 
     // Verify that we can read the data and it compiles with inferred types
     val rows = csv.toList
     assertEquals(rows.size, 2)
 
     // Test access to columns with the inferred types
-    assertEquals(csv.column["Doubles"].toList.head, 1.1) // Double
-    assertEquals(csv.column["Strings"].toList.head, "blah") // String
+    assertEquals(rows.column["Doubles"].toList.head, 1.1) // Double
+    assertEquals(rows.column["Strings"].toList.head, "blah") // String
     // Apache POI correctly identifies these as integers
-    assertEquals(csv.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
-    assertEquals(csv.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
   }
 
   test("excel provider all TypeInferrer variants now supported") {
@@ -163,32 +172,32 @@ class ExcelSuite extends munit.FunSuite:
   test("excel provider with TypeInferrer.FirstN should infer types automatically") {
     // Test FirstN with Numbers.xlsx - based on error message, types are inferred as:
     // (Doubles : Double, Int : Double, Longs : Double, Strings : String)
-    def csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FirstN(2))
+    val csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FirstN(2))
 
     // Verify that we can read the data and it compiles with inferred types
     val rows = csv.toList
     assertEquals(rows.size, 2)
 
     // Test access to columns with the actually inferred types
-    assertEquals(csv.column["Doubles"].toList.head, 1.1) // Double
-    assertEquals(csv.column["Strings"].toList.head, "blah") // String
+    assertEquals(rows.column["Doubles"].toList.head, 1.1) // Double
+    assertEquals(rows.column["Strings"].toList.head, "blah") // String
 
     // Apache POI correctly identifies these as integers based on cell type
-    assertEquals(csv.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
-    assertEquals(csv.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Int"].toList.head, 1) // Apache POI correctly identifies as Int
+    assertEquals(rows.column["Longs"].toList.head, 1) // Apache POI correctly identifies as Int
   }
 
   test("excel provider with TypeInferrer.FirstN and preferIntToBoolean parameter") {
     // Test FirstN with custom preferIntToBoolean setting - just verify compilation and basic access
-    def csv = Excel.resource("Numbers.xlsx", "Sheet1", "", TypeInferrer.FirstN(1, false))
+    val csv: ExcelIterator[("Doubles", "Int", "Longs", "Strings"), (Double, Int, Int, String)] = Excel.resource("Numbers.xlsx", "Sheet1", "A1:D3", TypeInferrer.FirstN(1, false))
 
     // Verify that we can read the data and it compiles with inferred types
     val rows = csv.toList
     assertEquals(rows.size, 2)
 
     // Basic functionality test - verify we can access data (don't assert specific types due to preferIntToBoolean differences)
-    assertEquals(csv.column["Doubles"].toList.head, 1.1)
-    assertEquals(csv.column["Strings"].toList.head, "blah")
+    assertEquals(rows.column["Doubles"].toList.head, 1.1)
+    assertEquals(rows.column["Strings"].toList.head, "blah")
     // Note: Int/Longs columns may be inferred differently with preferIntToBoolean=false
   }
 

scautable/test/src-jvm/testExcel.scala

@@ -97,7 +97,7 @@ class CSVWriterSuite extends munit.FunSuite:
 3,4,8
 5,6,9"""
 
-    assertEquals(data.toCsv(true, ',', '"'), expected)
+    assertNoDiff(data.toCsv(true, ',', '"'), expected)
   }
 
   test("List[NamedTuple].toCsv with empty values") {
@@ -112,7 +112,7 @@ class CSVWriterSuite extends munit.FunSuite:
 3,,8
 5,6,"""
 
-    assertEquals(data.toCsv(true, ',', '"'), expected)
+    assertNoDiff(data.toCsv(true, ',', '"'), expected)
   }
 
   test("Vector[NamedTuple].toCsv with numeric types") {
@@ -125,7 +125,7 @@ class CSVWriterSuite extends munit.FunSuite:
 1,Alice,95.5
 2,Bob,87.1"""
 
-    assertEquals(data.toCsv(true, ',', '"'), expected)
+    assertNoDiff(data.toCsv(true, ',', '"'), expected)
   }
 
   test("empty collection toCsv") {

scautable/test/src/csvWriterTest.scala

@@ -1,6 +1,5 @@
 # Excel
 
-
 Reading tables from excel should work a very similar way to CSV. The default behaviour is a bit of a hail mary. It assumes the excel workbook is rather well behaved, and that a blindly configured apache POI `RowIterator` will capture appropriate data. You should _not_ expect this method to be robust to blank rows / columns/ data elsewhere in the sheet.
 
 ```scala mdoc
@@ -25,3 +24,18 @@ One can also read from an absolute path
 import io.github.quafadas.table.*
 val csv = Excel.absolutePath("path/to/SimpleTable.xlsx", "Sheet1")
 ```
+
+## Problems and Hints
+
+It is strongly recommended to specify a complete range when working with Excel. e..g.
+
+`val range = Excel.resource("Numbers.xlsx", "Sheet1", "A1:C3", TypeInferrer.FromAllRows)`
+
+Although this _may_ work, 
+
+`val norange = Excel.resource("Numbers.xlsx", "Sheet1", TypeInferrer.FromAllRows)`
+`val norange = Excel.resource("Numbers.xlsx", "", TypeInferrer.FromAllRows)`
+
+Excel is somewhat pathological with regard to sheet boundaries. It is likely this will include blank cells, which will muck up type inference and reading ranges.
+
+In general, trhe implementation here is not robust to Excel's flexibility (reading formula's is unimplemented) and assumes that we are working with a simple, well formed table.