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

NIFI-14106 Fixed bug which led to loss of dates and times precision. #9718

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dan-s1
Copy link
Contributor

@dan-s1 dan-s1 commented Feb 12, 2025

Summary

NIFI-14106
As detailed in the ticket , I reverted the change made in #9466 as the org.apache.poi.ss.usermodel.CellCopyContext can be used to prevent exceeding the cell styles. In addition this PR involved having to correct a POI bug which I reported in the following thread and can be found here. As a result I had to copy the following methods from the following classes in order to fix the bug and make use of org.apache.poi.ss.usermodel.CellCopyContext.

  • copyRows from org.apache.poi.xssf.usermodel.XSSFSheet
  • copyRowFrom from org.apache.poi.xssf.usermodel.XSSFRow
  • copyCell from org.apache.poi.ss.util.CellUtil

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

@dan-s1 Can you include a link to the issue created for the POI project? This change includes copying a good bit of code, and the particular elements that required modification are not clear. On a cursory read, if we proceed with this approach, it seems like there are some aspects of the methods that are not needed.

@dan-s1
Copy link
Contributor Author

dan-s1 commented Feb 12, 2025

@exceptionfactory Is what I included now in the description enough?
Based on the included thread, I understood that the POI folks would make the necessary changes although I am not sure when that will be hence I included the necessary changes in SplitExcel.

@exceptionfactory
Copy link
Contributor

@exceptionfactory Is what I included now in the description enough? Based on the included thread, I understood that the POI folks would make the necessary changes although I am not sure when that will be hence I included the necessary changes in SplitExcel.

Thanks for providing the additional background and link, that is helpful.

I'm a bit concerned about the level of complexity from these additional methods, but I haven't walked through the details closely. I will plan on reviewing soon.

@mosermw
Copy link
Member

mosermw commented Feb 21, 2025

When copying code from another project, one must include a note to that effect in the NOTICE file the NAR, correct?

However, I would be -1 on copying POI code into NiFi, when we could instead simply wait for a POI release that has the desired behavior.

@exceptionfactory
Copy link
Contributor

When copying code from another project, one must include a note to that effect in the NOTICE file the NAR, correct?

However, I would be -1 on copying POI code into NiFi, when we could instead simply wait for a POI release that has the desired behavior.

I agree with you @mosermw, it looks like there has already been some progress on this issue in the POI project, so it seems better to wait for those changes to be completed and released.

@exceptionfactory
Copy link
Contributor

@dan-s1 It looks like the changes in POI in the following commit and several others related would obviate the changes in this pull request:

apache/poi@f61ddde

With the work there, it seems best to close this pull request and track the POI version changes when they are ready.

What do you think?

@dan-s1
Copy link
Contributor Author

dan-s1 commented Feb 24, 2025

@exceptionfactory Thanks for showing me that latest commit as that was the change I was waiting for to allow for reverting the changes of #9466. It seems the POI changes would solve all the issues I had so I am fine with closing this PR. Should I put something in a comment on the ticket so we know to watch for this?

@exceptionfactory
Copy link
Contributor

@dan-s1, Yes, a comment in the Jira, linking to the POI commit and mentioning that this should be resolved in the next version would be helpful.

@dan-s1
Copy link
Contributor Author

dan-s1 commented Feb 24, 2025

@exceptionfactory Okay I added that. Is there anyway to put the ticket on hold in Jira?

@exceptionfactory
Copy link
Contributor

I'm not seeing a blocked status option in Jira, but that's fine, the comment is sufficient for now.

@dan-s1
Copy link
Contributor Author

dan-s1 commented Feb 24, 2025

@exceptionfactory I just wanted to add even after the latest POI changes, there is still a need to revert the changes made in #9466 to allow cell styles to be copied which would enable copying of date formats. I think this ticket could cover that. Is that okay or should another ticket be created?

@exceptionfactory
Copy link
Contributor

@exceptionfactory I just wanted to add even after the latest POI changes, there is still a need to revert the changes made in #9466 to allow cell styles to be copied which would enable copying of date formats. I think this ticket could cover that. Is that okay or should another ticket be created?

Sure, this ticket can cover those changes.

@dan-s1 dan-s1 closed this Feb 24, 2025
@dan-s1 dan-s1 reopened this Mar 17, 2025
@dan-s1
Copy link
Contributor Author

dan-s1 commented Mar 17, 2025

@exceptionfactory I encapsulated the copy rows logic in a private static class as you had suggested and I updated the NOTICE file accordingly.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the updated approach @dan-s1. I plan to take a closer look soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants