Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support read unstructured excel file #901

Merged
merged 6 commits into from
Oct 31, 2024

Conversation

khm0651
Copy link

@khm0651 khm0651 commented Oct 1, 2024

It has been confirmed that the current Dataframe.readExcel function only supports structured Excel formats.

Of course, it would be ideal if everyone created Excel files in a structured format, but as shown in the image, when dealing with unstructured Excel formats, the current DataFrame approach of always designating the first row as the header causes difficulties in usage.

The Python pandas library supports this, making it very efficient to use.

As a result, for unstructured Excel formats, i've implemented support by using a value called withDefaultHeader.

When set to true, it automatically generates headers using NameRepairStrategy,
thus enabling support for unstructured Excel formats.

when withDefaultHeader is set to true, it operates as [NameRepairStrategy.MAKE_UNIQUE]

Is there a better approach?

image

@khm0651 khm0651 force-pushed the support_unstructured_excel_file branch from 301f53c to 2b3361f Compare October 1, 2024 12:25
@Jolanrensen Jolanrensen self-requested a review October 1, 2024 12:49
@Jolanrensen
Copy link
Collaborator

Thanks for your contribution!

Some thoughts:

So if I'm correct, withDefaultHeader = true will make the columns be named according to excel columns, like "A", "B", "C" etc.? It's not really clear to me this will happen from your description of the argument.

I might change it to firstRowIsHeader = true as the argument, then describing that when that's set to false explicitly, DF will fall-back to excel letter column names, else it will take the first row (after skipRows) as the header.

Also, am I correct that in the current implementation, column J will not be included in the result?
image

@khm0651
Copy link
Author

khm0651 commented Oct 1, 2024

Oh, it's a bit different. When withDefaultHeader is set to true, it doesn't use the row specified by skipRow as the header. If skipRow is set, it retrieves data starting from the row specified by skipRow, using automatically generated headers.
Excel columns are automatically generated as "A", "B", "C", etc.. and so on.

However, as @Jolanrensen pointed out, I hadn't considered that particular case.
It seems we need to address this case as well
to accommodate this case, should calculate and incorporate a maximum value.

and i agree change it to firstRowIsHeader = true as the argument too

"There are no defined cells on header row number ${skipRows + 1} (1-based index). Pass `columns` argument to specify what columns to read or make sure the index is correct"

else -> {
val notEmptyRow = sheet.rowIterator().asSequence().maxByOrNull { it.lastCellNum }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might name this largestRow or something like that. Good solution though :) didn't know about lastCellNum

@@ -103,11 +107,21 @@ public fun DataFrame.Companion.readExcel(
stringColumns: StringColumns? = null,
rowsCount: Int? = null,
nameRepairStrategy: NameRepairStrategy = NameRepairStrategy.CHECK_UNIQUE,
firstRowIsHeader: Boolean = true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently learned about bytecode compatibility which will break when adding this extra argument (https://kotlinlang.org/docs/api-guidelines-backward-compatibility.html#avoid-adding-arguments-to-existing-api-functions). This means that when you use a library that depends on DataFrame 0.14 while your own project depends on DataFrame 0.15 and that library calls readExcel(), you'll get a NoSuchMethodError. We're adding a detector plugin for these cases.
We can solve this, however, by adding some extra DeprecationLevel.HIDDEN overloads of the old version of these functions. I could do this for you if you like :) The rest looks good, so afterwards we could merge it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the review and for accepting my answer!
I truly appreciate it.

@Jolanrensen Jolanrensen added the enhancement New feature or request label Oct 30, 2024
@Jolanrensen Jolanrensen added this to the 0.15.0 milestone Oct 30, 2024
@Jolanrensen Jolanrensen force-pushed the support_unstructured_excel_file branch from e53634e to b6fe681 Compare October 31, 2024 14:22
@Jolanrensen Jolanrensen merged commit 77b2f56 into Kotlin:master Oct 31, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants