Copilot commented on code in PR #674: URL: https://github.com/apache/fesod/pull/674#discussion_r2490362539
########## fesod/src/test/java/org/apache/fesod/excel/head/HeaderMergeStrategyTest.java: ########## @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.excel.head; + +import java.io.File; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.fesod.excel.FastExcel; +import org.apache.fesod.excel.enums.HeaderMergeStrategy; +import org.apache.fesod.excel.util.TestFileUtil; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.util.CellRangeAddress; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; + +/** + * Test for header merge strategy + * + */ +@TestMethodOrder(MethodOrderer.MethodName.class) +public class HeaderMergeStrategyTest { + + private static File fileNone; + private static File fileHorizontalOnly; + private static File fileVerticalOnly; + private static File fileFullRectangle; + private static File fileAuto; + + @BeforeAll + public static void init() { + fileNone = TestFileUtil.createNewFile("headerMergeStrategyNone.xlsx"); + fileHorizontalOnly = TestFileUtil.createNewFile("headerMergeStrategyHorizontalOnly.xlsx"); + fileVerticalOnly = TestFileUtil.createNewFile("headerMergeStrategyVerticalOnly.xlsx"); + fileFullRectangle = TestFileUtil.createNewFile("headerMergeStrategyFullRectangle.xlsx"); + fileAuto = TestFileUtil.createNewFile("headerMergeStrategyAuto.xlsx"); + } + + @Test + public void testNoneStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileNone) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.NONE) + .sheet() + .doWrite(createTestData()); + + // Verify no merged regions + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileNone)) { + Sheet sheet = workbook.getSheetAt(0); + Assertions.assertEquals( + 0, sheet.getNumMergedRegions(), "NONE strategy should not create any merged regions"); + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testHorizontalOnlyStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileHorizontalOnly) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.HORIZONTAL_ONLY) + .sheet() + .doWrite(createTestData()); + + // Verify only horizontal merges exist + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileHorizontalOnly)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be horizontal only (same row) + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + Assertions.assertEquals( + region.getFirstRow(), + region.getLastRow(), + "HORIZONTAL_ONLY strategy should only merge cells in the same row"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testVerticalOnlyStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileVerticalOnly) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.VERTICAL_ONLY) + .sheet() + .doWrite(createTestData()); + + // Verify only vertical merges exist + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileVerticalOnly)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be vertical only (same column) + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + Assertions.assertEquals( + region.getFirstColumn(), + region.getLastColumn(), + "VERTICAL_ONLY strategy should only merge cells in the same column"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testFullRectangleStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileFullRectangle) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.FULL_RECTANGLE) + .sheet() + .doWrite(createTestData()); + + // Verify all merged regions form valid rectangles + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileFullRectangle)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be valid rectangles + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + // Verify rectangle is valid (not just a single cell) + Assertions.assertTrue( + region.getFirstRow() != region.getLastRow() + || region.getFirstColumn() != region.getLastColumn(), + "Merged region should not be a single cell"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testAutoStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileAuto) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.AUTO) + .sheet() + .doWrite(createTestData()); + + // AUTO strategy should work similar to the old behavior + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileAuto)) { + Sheet sheet = workbook.getSheetAt(0); + // Just verify that the file was created successfully + Assertions.assertNotNull(sheet, "Sheet should be created"); + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + /** + * Create test head data with mergeable cells + */ + private List<List<String>> createTestHead() { + List<List<String>> head = new ArrayList<>(); + // Row 0: ["A", "A", "B"] + head.add(new ArrayList<>(Arrays.asList("A"))); + head.add(new ArrayList<>(Arrays.asList("A"))); + head.add(new ArrayList<>(Arrays.asList("B"))); + // Row 1: ["A1", "A2", "B1"] Review Comment: The comment describing the head structure is misleading. The comments say 'Row 0' and 'Row 1', but the data structure represents columns, not rows. Each inner list represents a column's header hierarchy. The first three entries create 3 columns where the first row has 'A', 'A', 'B', and the next three entries add a second row underneath with 'A1', 'A2', 'B1'. The comments should clarify this is column-based data, e.g., 'Column 0-2 row 0' and 'Columns 0-2 with row 0 and row 1'. ```suggestion // Columns 0-2 with row 0: ["A"], ["A"], ["B"] head.add(new ArrayList<>(Arrays.asList("A"))); head.add(new ArrayList<>(Arrays.asList("A"))); head.add(new ArrayList<>(Arrays.asList("B"))); // Columns 0-2 with row 0 and row 1: ["A", "A1"], ["A", "A2"], ["B", "B1"] ``` ########## fesod/src/main/java/org/apache/fesod/excel/write/property/ExcelWriteHeadProperty.java: ########## @@ -125,37 +144,146 @@ public List<CellRange> headCellRangeList() { String headName = headNameList.get(j); int lastCol = i; int lastRow = j; - for (int k = i + 1; k < headList.size(); k++) { - String key = k + "-" + j; - if (headList.get(k).getHeadNameList().get(j).equals(headName) && !alreadyRangeSet.contains(key)) { - alreadyRangeSet.add(key); - lastCol = k; - } else { - break; + + // Horizontal merge (if allowed by strategy) + if (mergeStrategy == HeaderMergeStrategy.HORIZONTAL_ONLY + || mergeStrategy == HeaderMergeStrategy.FULL_RECTANGLE + || mergeStrategy == HeaderMergeStrategy.AUTO) { + for (int k = i + 1; k < headList.size(); k++) { + String key = k + "-" + j; + if (headList.get(k).getHeadNameList().size() > j + && Objects.equals( + headList.get(k).getHeadNameList().get(j), headName) + && !alreadyRangeSet.contains(key)) { + alreadyRangeSet.add(key); + lastCol = k; + } else { + break; + } } } + + // Vertical merge (if allowed by strategy) Set<String> tempAlreadyRangeSet = new HashSet<>(); - outer: - for (int k = j + 1; k < headNameList.size(); k++) { - for (int l = i; l <= lastCol; l++) { - String key = l + "-" + k; - if (headList.get(l).getHeadNameList().get(k).equals(headName) - && !alreadyRangeSet.contains(key)) { - tempAlreadyRangeSet.add(l + "-" + k); + if (mergeStrategy == HeaderMergeStrategy.VERTICAL_ONLY + || mergeStrategy == HeaderMergeStrategy.FULL_RECTANGLE + || mergeStrategy == HeaderMergeStrategy.AUTO) { + outer: + for (int k = j + 1; k < headNameList.size(); k++) { + // For FULL_RECTANGLE and AUTO, verify all cells in the row + boolean canMerge = true; + for (int l = i; l <= lastCol; l++) { + String key = l + "-" + k; + if (headList.get(l).getHeadNameList().size() <= k + || !Objects.equals( + headList.get(l).getHeadNameList().get(k), headName) + || alreadyRangeSet.contains(key)) { + canMerge = false; + break; + } + } + + // For AUTO strategy, also check context consistency + if (canMerge && mergeStrategy == HeaderMergeStrategy.AUTO) { + canMerge = canMergeVertically(headList, j, k, i, lastCol, headName); + } + + if (canMerge) { + for (int l = i; l <= lastCol; l++) { + String key = l + "-" + k; + tempAlreadyRangeSet.add(key); + } + lastRow = k; } else { break outer; } } - lastRow = k; alreadyRangeSet.addAll(tempAlreadyRangeSet); } - if (j == lastRow && i == lastCol) { - continue; + + // For FULL_RECTANGLE strategy, verify the entire rectangle is valid + if (mergeStrategy == HeaderMergeStrategy.FULL_RECTANGLE) { + if (!isValidRectangleRegion(headList, j, lastRow, i, lastCol, headName)) { + // If rectangle is invalid, fall back to single cell (no merge) + continue; + } + } + + // Add merge range if it's larger than a single cell + if (j != lastRow || i != lastCol) { + cellRangeList.add(new CellRange( + j, + lastRow, + head.getColumnIndex(), + headList.get(lastCol).getColumnIndex())); } - cellRangeList.add(new CellRange( - j, lastRow, head.getColumnIndex(), headList.get(lastCol).getColumnIndex())); } } return cellRangeList; } + + /** + * Check if two rows can be merged vertically based on context consistency + * + * @param headList The list of heads + * @param row1 First row index + * @param row2 Second row index + * @param startCol Start column index + * @param endCol End column index + * @param cellName The cell name to merge + * @return true if the rows can be merged + */ + private boolean canMergeVertically( + List<Head> headList, int row1, int row2, int startCol, int endCol, String cellName) { + // Check if there's a row above that provides context + if (row1 > 0) { + // Check if all cells in the range have the same context above + for (int col = startCol; col <= endCol; col++) { + boolean hasUpper1 = headList.get(col).getHeadNameList().size() > row1; + boolean hasUpper2 = headList.get(col).getHeadNameList().size() > row2; + + // If one row has upper context but the other doesn't, don't merge + if (hasUpper1 != hasUpper2) { + return false; + } + + if (hasUpper1) { + String upper1 = headList.get(col).getHeadNameList().get(row1 - 1); + String upper2 = headList.get(col).getHeadNameList().get(row2 - 1); + // If context (upper cells) is different, don't merge + if (!Objects.equals(upper1, upper2)) { + return false; + } + } + } + } + return true; + } Review Comment: The parameter `cellName` is declared but never used in this method. If it's not needed for the logic, it should be removed from the method signature to improve code clarity. ########## fesod/src/test/java/org/apache/fesod/excel/head/HeaderMergeStrategyTest.java: ########## @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.excel.head; + +import java.io.File; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.fesod.excel.FastExcel; +import org.apache.fesod.excel.enums.HeaderMergeStrategy; +import org.apache.fesod.excel.util.TestFileUtil; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.util.CellRangeAddress; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; + +/** + * Test for header merge strategy + * + */ +@TestMethodOrder(MethodOrderer.MethodName.class) +public class HeaderMergeStrategyTest { + + private static File fileNone; + private static File fileHorizontalOnly; + private static File fileVerticalOnly; + private static File fileFullRectangle; + private static File fileAuto; + + @BeforeAll + public static void init() { + fileNone = TestFileUtil.createNewFile("headerMergeStrategyNone.xlsx"); + fileHorizontalOnly = TestFileUtil.createNewFile("headerMergeStrategyHorizontalOnly.xlsx"); + fileVerticalOnly = TestFileUtil.createNewFile("headerMergeStrategyVerticalOnly.xlsx"); + fileFullRectangle = TestFileUtil.createNewFile("headerMergeStrategyFullRectangle.xlsx"); + fileAuto = TestFileUtil.createNewFile("headerMergeStrategyAuto.xlsx"); + } + + @Test + public void testNoneStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileNone) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.NONE) + .sheet() + .doWrite(createTestData()); + + // Verify no merged regions + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileNone)) { + Sheet sheet = workbook.getSheetAt(0); + Assertions.assertEquals( + 0, sheet.getNumMergedRegions(), "NONE strategy should not create any merged regions"); + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testHorizontalOnlyStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileHorizontalOnly) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.HORIZONTAL_ONLY) + .sheet() + .doWrite(createTestData()); + + // Verify only horizontal merges exist + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileHorizontalOnly)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be horizontal only (same row) + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + Assertions.assertEquals( + region.getFirstRow(), + region.getLastRow(), + "HORIZONTAL_ONLY strategy should only merge cells in the same row"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testVerticalOnlyStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileVerticalOnly) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.VERTICAL_ONLY) + .sheet() + .doWrite(createTestData()); + + // Verify only vertical merges exist + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileVerticalOnly)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be vertical only (same column) + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + Assertions.assertEquals( + region.getFirstColumn(), + region.getLastColumn(), + "VERTICAL_ONLY strategy should only merge cells in the same column"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testFullRectangleStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileFullRectangle) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.FULL_RECTANGLE) + .sheet() + .doWrite(createTestData()); + + // Verify all merged regions form valid rectangles + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileFullRectangle)) { + Sheet sheet = workbook.getSheetAt(0); + int mergedRegionCount = sheet.getNumMergedRegions(); + + // All merged regions should be valid rectangles + for (int i = 0; i < mergedRegionCount; i++) { + CellRangeAddress region = sheet.getMergedRegion(i); + // Verify rectangle is valid (not just a single cell) + Assertions.assertTrue( + region.getFirstRow() != region.getLastRow() + || region.getFirstColumn() != region.getLastColumn(), + "Merged region should not be a single cell"); + } + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + @Test + public void testAutoStrategy() { + List<List<String>> head = createTestHead(); + FastExcel.write(fileAuto) + .head(head) + .headerMergeStrategy(HeaderMergeStrategy.AUTO) + .sheet() + .doWrite(createTestData()); + + // AUTO strategy should work similar to the old behavior + try (org.apache.poi.ss.usermodel.Workbook workbook = + org.apache.poi.ss.usermodel.WorkbookFactory.create(fileAuto)) { + Sheet sheet = workbook.getSheetAt(0); + // Just verify that the file was created successfully + Assertions.assertNotNull(sheet, "Sheet should be created"); + } catch (Exception e) { + throw new RuntimeException("Failed to verify merged regions", e); + } + } + + /** + * Create test head data with mergeable cells + */ + private List<List<String>> createTestHead() { + List<List<String>> head = new ArrayList<>(); + // Row 0: ["A", "A", "B"] + head.add(new ArrayList<>(Arrays.asList("A"))); + head.add(new ArrayList<>(Arrays.asList("A"))); + head.add(new ArrayList<>(Arrays.asList("B"))); + // Row 1: ["A1", "A2", "B1"] + head.add(new ArrayList<>(Arrays.asList("A", "A1"))); + head.add(new ArrayList<>(Arrays.asList("A", "A2"))); + head.add(new ArrayList<>(Arrays.asList("B", "B1"))); + return head; + } + + /** + * Create test data + */ + private List<List<Object>> createTestData() { + List<List<Object>> data = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + data.add(new ArrayList<>(Arrays.asList("A" + i, "B" + i, "C" + i))); Review Comment: The test creates a head structure with 6 columns (indices 0-5 based on the createTestHead method adding 6 lists), but createTestData only provides 3 data values per row. This mismatch will likely cause issues during write operations or result in incomplete data. The data array should have 6 values to match the 6-column header structure. ```suggestion data.add(new ArrayList<>(Arrays.asList("A" + i, "B" + i, "C" + i, "D" + i, "E" + i, "F" + i))); ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
