formula optimization - #8304
formula optimization#8304rmannibucau wants to merge 7 commits into
Conversation
|
Nice improvement! I will take a look and make some tests with some pipelines. |
|
I am not a big fan of this idea, we could make it more clear that this is an excel tranform and slow. We will end up having to create/implement strange behavior POI/Excel has to make it compatible with it. |
|
I agree with @hansva. Making this fully POI compliant is not realistic, let alone maintaining it and remaining compatible. This could be great as a separate high-performance subset of the formulas, but it needs to be very clear that this is a very different transform behind the scenes. |
|
In the absolute I agree with you but mean we migrate on the fly formula between excel and this new component in pipelines too cause you can't ask people to rewrite hundreds of pipelines IMHO. Would it be ok to:
? Indeed it makes a 2 step thing but it is always the case for huge changes like that so sounds the most profitable for end users to me. wdyt? |
|
Imho, we should at the very minimum have a way to let users choose which engine they want to use to process their formulas, either POI or the fast engine. There currently are a couple of supported functions, let's say that grows to ~20, how would users currently know which engine processes their formula? We should also make sure that we have integration tests that prove that the POI and "fast" formula engines provide the same results, or have the differences in formula behavior documented. Don't get me wrong, I appreciate the initiative and the effort, but as you say, this will be a huge change if done well, so we need to be cautious. |
this PR is designed to ensure the user doesn't care, formula component is not designed around poi but more excel, the PR implement formula it can in a fast path else falls back on poi so it is transparent (else it is a bug and the system property a workaround) so the minimum spirit of this PR is to not have to get this question.
it is in the PR -> https://github.com/rmannibucau/hop/blob/38a644a4e34d084524084589d5670340fcce713e/plugins/transforms/formula/src/test/java/org/apache/hop/pipeline/transforms/formula/FormulaFastPathParityTest.java , not sure what an integration test would bring there but coverage is there
100% aligned and this is why there is a system property backdoor, the question is more are we cautious at the cost of not enabling existing user to rely on it and only enable new users (or costly migration/test/review - note that there is it mainly human and not tech) or just make it work OOTB. I prefer the upgrade and it works for free option and put effort in the parser harnessing on my side. |
Code Review of PR #8304: Formula OptimizationGreat initiative! Bypassing Apache POI's HSSF/XSSF workbook and sheet allocation for straightforward expressions is a huge win for one of Hop's most heavily used transforms. The 10x–20x microbenchmark improvements demonstrate how much overhead POI introduces for row-by-row formula evaluation. Below are a few critical correctness and runtime failure issues, Excel/POI semantic discrepancies, and performance suggestions to address before this can safely merge. 1. Critical Bugs & Runtime Failure Risks1.1
|
|
May be having a new transform plugin from this PR can be enough and if a user want to migrate from actual POI to this one, he can just be changing the name in the xml project file or we can provide a migrate script? |
|
@fpapon how do you explain to end users there are two transforms doing exactly the same thing but one is slow "because we can"? this is where I think the technical option fails facing end users expectations |
|
@rmannibucau The main problem with the PR as it stands imo is that the output of the "fast" and POI modes won't be identical (this is also Point 2 in @mattcasters's review). |
Follow-up review of PR #8304 (head
|
|
the goal is really the parity without the cost to create workbook, xml still etc. that said the failure case is guarded with the system property, so the workaround is quick to spot and ack we should highlight this change in the changelog. /me will check the other diff now |
|
I personally still hate the idea of spending time on getting POI parity, where we could just create a new formula transform type that could have cool new functions and formulas that wouldn't be covered by POI. If your issue is Pentaho migration, you can just revive the old code outside of an Apache repository like @mattcasters did here Edit: well that's also a re-implementation, technically you could just port the code |
|
On top of that: we'd need real integration tests that run pipeline for the formulas in both fast and POI mode to make sure they both produce the exacte same result for every individual function, combination of functions etc. |
note on that: it is built in since first commit
partly but the costly part of POI is not the computation, itis all the rest and the fact we can support partly and still be 100% accurate with the POI fallback means we can get really significant boost for low investment (this PR is literally an blocker -> enabler game changer for Apache Hop adoption) the "other"/new component is just way too hard to understand IMHO, in the UI when you will select a transform you will have "formula or formula", best case you get "slow but complete formula VS fast but partial formula". I don't see how it can be defended from an UX/end user perspective so I'm very hesitating to go that route.
guess you know the story there, it is built-in or it is custom and more you pull custom code less you need the built in, so trying to push strong on the standard Apache Hop solution there. More on a technical aspect there is no real technical justification (I understand the licensing etc) to use Apache POI for data but small ones so think this critical component should get more love. Now, if you all converge to say me we deprecate the Apache POI component in next minor and clearly state we move to the "new" one and drop the POI one in next major then I will totally align on creating a new one, if not I don't see splitting as positive for end users. (sorry for the big post) edit: if @mattcasters you want to push back your component to hop and integrate it to formula component I'm super happy to drop that PR too, just want a "not slow" impl by default edit 2: i'll check if we can propose to Apache POI a new API we could leverage to converge too in the mean time |
We have hundreds of integration tests in the integration-tests folder. These are Apache Hop pipelines and workflows to test Apache Hop functionality. These have already uncovered tons of bugs that slipped through the cracks in code-based tests and have prevented numerous regressions.
I think the overall tone of the discussion is actually the exact opposite. I can't imagine POI disappearing from Hop in the coming years. |
|
You can always make a distinction between "Excel formulas" and "Hop formulas" or other wording; I don't see an issue with competing transforms to reach the same goal or even other goals. We also have user-defined Java expressions; you could write your formula in JavaScript, Groovy, or use the calculator. My main thing is, what if you want to create formulas to for example do IP address calculations on our INET type, what if you want to use values from inside a JSON type. It can't be added to this implementation so at some point a competing implementation will arrive. |
|
And tbh, I am not against deprecating that transform. It was created as a best-effort stopgap to provide an Apache licensed way forward from the Pentaho formula transform. It has major flaws, Excel formulas have major flaws, especially around date calculations and the fact that return types are not consistent. |
|
Technically, Pentaho libformula implements the The main blockers for using the libformula library from Pentaho were the unmaintained status and age as well as the LGPL license on top of it. To unblock at least the second part I had Gemini rewrite the library from scratch against the spec here. I's a green-field re-implementation so brought back to APL2 and can be further maintained under Apache Hop. @rmannibucau I would not abandon the progress you made just like that. The arguments agains maintaining a separate stable set of formula is weak because it's essentially just bug fixing work. For the migration work I can bring the APL2 OpenFormula variant into the Hop project as a drop-in replacement for the PDI/Kettle formula step. This should allow migrations to continue while we see if we can fix the last couple of compatibility issues with the work Romain has been doing. I'm happy to help out either way. It's all for the best. |
well there it is super localized and 100% computation related so think the test of this PR is more than covering the change, no? there is no real way to discover something you didnt spot without the UI using the UI, would just be slower. Do you have something specific in mind? @hansva my issue with competing solutions is that it is hard to have a proper communication to end users and it promotes repeated migrations or a fragmented ecosystem, both are negative on user end side (even if I get the core issues it solves but on my side I care more of end users than core challenges). @mattcasters think bringing the component and flagging it "PDI compatibility" somehow (likely in the name and migration tool) would be super beneficial. Remains the question of the main promoted formula component outside the migration scope. I also had another idea, maybe a bit more crazy but worth giving a try since it shouldn't be too much work (but not 100% sure it pays as much): just implementing an excel-file-less formula evaluator in Apache POI si Apache hop gets it almost for free. Will try to give it a try and report there - at least to say "it was literally too crazy" if it didnt go anywhere to avoid another one to give a try later on. edit: here is the Apache POI initial proposal apache/poi#1271 and main...rmannibucau:hop:dev/poi-standalone-api |
| // it, so the NA sentinel short-circuits the whole expression. | ||
| return FastFormulaCompiler.NA; | ||
| } | ||
| switch (op) { |
There was a problem hiding this comment.
Hmm, can you refine what you have in mind? in terms of LoC it is mainly a style thing (you only gain return keyword more or less and take the risk to be slower - have a list of "if" then a switch with refactors) so not sure I would chane it the way you do think
|
@fpapon @mattcasters know you did exchange about the formula component, wonder if there is any convergence yet? should I close this one? |
|
Last time I checked, there was no complete parity between the functions. We are however missing documentation on which functions follow the fast path and which don't |
Did try to refine it, the side note will be that Apache POI is breaking that as well (in particular on numbers) for the next big upgrade.
one of the intent to do it this way was to not need it and get it for free when pormoted think the flag was address with https://github.com/apache/hop/pull/8304/changes#diff-b907def2fb744ff620138e0b8a090f90fac018e8fa7e53215776615c9d26080bR141 until I misunderstood the point |
|
Add it to the documentation, then the users at least have a pointer on how to disable it. And which functions are impacted. But honestly, the -D option isn't the way to go, our goal is to have every option editable via the GUI |
Idea is to try to bypass Apache POI for formula evaluation.
Since it can be a crazy huge task the philosophy is "can i optimize this formula, if yes do it", there is also a system property to disable/enable this feature until we are sure we don't need it anymore.
Microbenchmark
us/op, lower is better).-Xmx2g.-Dorg.apache.hop.pipeline.transforms.formula.fast.FastFormulaCompiler.enabled=false.Formula.processRow()(argument resolution + evaluation + output)The scenarios below use formulas that the fast evaluator supports (so a fast vs POI difference is observable).
"CREATE TABLE " & [tableName] & "(FILENAME VARCHAR2(255), ROWNUM NUMBER, " & [sqlContent] & " )" & IF([compress]="Y"," COMPRESS","")[heures]*60 + [minutes]"merge into " & [outputTable] & " INPUT_TABLE using " & [inputTable] & " STAGING_TABLE ON (" & [joinClause] & " ) WHEN NOT MATCHED THEN INSERT ( " & [inputCols] & " ) VALUES ( STAGING_TABLE." & [outputCols] & "),"IF(OR([mycolumn]="Œ",[mycolumn]="1",[mycolumn]="3",[mycolumn]="4"),"1",IF([mycolumn]="5","0",[mycolumn]))[abcd] & [version] & [fghij] & [hhmmss] & "." & [ext]IF([somecolumn] <> "N"," AND WHAT = '1111111100000000'","") & IF([othercolumn] <> "N"," AND EVER != '1'","") & IF([abcdef] <> "null"," AND DUMMY IN (" & [ghij] & ")","")Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean install apache-rat:checkto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.addresses #123), if applicable.To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.