[Bug 61033] New: [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 61033] New: [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

            Bug ID: 61033
           Summary: [PATCH] Add Workbook.setCellFormulaValidation to
                    control whether formulas are validated during
                    Cell.setCellFormula or not
           Product: POI
           Version: unspecified
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: SS Common
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

Created attachment 34946
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34946&action=edit
patch

Our application must set formulas, then evaluate them.  Cell.setCellFormula
does expensive formula parsing, then FormulaEvaluator.evaluate() parses it
again.  With this patch if we turn off formula validation our total runtime per
job almost halves, dropping from 21 minutes to 11 minutes.

Let me know if anything should change.  I implemented this as part of the
Workbook interface, but then realized that HSSFWorkbook and SXSSFWorkbook
actually don't parse the formula or throw the ParseException anyway, they might
in the future though so I guess this is still the correct place?

Thanks much!

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

Travis Burtrum <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34946|patch                       |0001-Add-Workbook.setCellFo
        description|                            |rmulaValidation-to-control-
                   |                            |whe.patch

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #1 from Javen O'Neal <[hidden email]> ---
Could you please add some unit tests covering the new functionality?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #2 from Travis Burtrum <[hidden email]> ---
It's basically just a setter/getter for a boolean, and then not calling
validation logic if false, I'm not sure of a great way to unit test that,
especially since it only applies to XSSF too?

I guess maybe I could set it false and send in an invalid formula and assert I
didn't get an exception?  Anything else?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Greg Woolsey
Test the positive case too, true + invalid formula allowed, test that the
default is true, test that flipping it from false to true with an invalid
formula in place, resulting in an invalid workbook, is somehow handled so
users don't get odd errors later. Maybe validate all formulas then?  That
would be expensive.  If we just allow an invalid state then, the javadoc
needs to clearly note that for the boolean setter and getter, and the
formula setter and getter.

It will make it easy for users to get things all mixed up but appear valid,
so the docs need to be vocal about it.

On Tue, Apr 25, 2017, 08:10 <[hidden email]> wrote:

> https://bz.apache.org/bugzilla/show_bug.cgi?id=61033
>
> --- Comment #2 from Travis Burtrum <[hidden email]> ---
> It's basically just a setter/getter for a boolean, and then not calling
> validation logic if false, I'm not sure of a great way to unit test that,
> especially since it only applies to XSSF too?
>
> I guess maybe I could set it false and send in an invalid formula and
> assert I
> didn't get an exception?  Anything else?
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #3 from Javen O'Neal <[hidden email]> ---
From Greg Woolsey on dev@:
Test the positive case too, true + invalid formula allowed, test that the
default is true, test that flipping it from false to true with an invalid
formula in place, resulting in an invalid workbook, is somehow handled so
users don't get odd errors later. Maybe validate all formulas then?  That
would be expensive.  If we just allow an invalid state then, the javadoc
needs to clearly note that for the boolean setter and getter, and the
formula setter and getter.

It will make it easy for users to get things all mixed up but appear valid,
so the docs need to be vocal about it.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #4 from Javen O'Neal <[hidden email]> ---
Combinations that need tested:

1. Evaluate formulas=false should not raise an exception when setting an
invalid formula.
2. Evaluate formulas=false should not raise an exception when setting a valid
formula.
3. Evaluate formulas=true should raise an exception when setting an invalid
formula.
4. Evaluate formulas=true should not raise an exception when setting a valid
formula.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #5 from Travis Burtrum <[hidden email]> ---
Created attachment 34953
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34953&action=edit
0001-Add-XSSFWorkbook.setCellFormulaValidation-to-control.patch

Alternative patch suggested by kiwiwingsā€ˇ on IRC to limit this addition to
XSSFWorkbook since neither HSSF or SXSSF ever validate the formula or throw
this exception anyway.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #6 from Travis Burtrum <[hidden email]> ---
Created attachment 34954
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=34954&action=edit
0002-Unit-tests-for-XSSFWorkbook.setCellFormulaValidation.patch

Unit tests for either of the previous patches.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

--- Comment #7 from Travis Burtrum <[hidden email]> ---
Also I've been pushing up my branches here too if it makes it easier to look
at:

https://github.com/moparisthebest/poi/commits/formula_patch
https://github.com/moparisthebest/poi/commits/formula_patchv2

v2 is the patch to only XSSFWorkbook, the first patches Workbook

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |PatchAvailable

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61033] [PATCH] Add Workbook.setCellFormulaValidation to control whether formulas are validated during Cell.setCellFormula or not

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61033

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Dominik Stadler <[hidden email]> ---
Applied via r1809071, thanks for providing the implementation and tests.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]